Skip to content

Implement Actions API. - #459

Merged
andreasabel merged 38 commits into
haskell-github:masterfrom
K0Te:github-actions-2
Jun 24, 2023
Merged

Implement Actions API.#459
andreasabel merged 38 commits into
haskell-github:masterfrom
K0Te:github-actions-2

Conversation

@K0Te

@K0TeK0Te commented Jun 22, 2021

Copy link
Copy Markdown
Contributor

Closes#458

Hi @phadej ,

I've implemented just ~5% of Actions API at the moment. I have many questions and need your guidance before proceeding further:

{
"artifacts": [
...
],
"total_count": 13676
}

I see three ways of handling this:

  1. A separate Response type for each list endpoint, it's simple but need a lot of code:
dataArtifactList=ArtifactList{artifactListArtifacts::!(VectorArtifact)
, artifactListTotalCount::!Int}deriving (Show, Data, Typeable, Eq, Ord, Generic
  1. Tag as a type-level symbol, this looks nicer, but needs extensions and moves tag towards Endpoints instead of Data:
dataPaginatedWithTotalCounta (tag::Symbol) =PaginatedWithTotalCount{paginatedWithTotalCountItems::!(Vectora)
, paginatedWithTotalCountTotalCount::!Int}
  1. Tag defined in instance declaration for each paginated type. This approach seems best, but might break if GitHub API uses different tags for the same type - in this case, the library would need new types, which will be confusing...
dataWithTotalCounta=WithTotalCount{withTotalCountItems::!(Vectora)
, withTotalCountTotalCount::!Int}deriving (Show, Data, Typeable, Eq, Ord, Generic)
instanceSemigroup (WithTotalCounta) where
(WithTotalCount items1 count1) <> (WithTotalCount items2 _) =WithTotalCount (items1 <> items2) count1
instanceFoldableWithTotalCountwherefoldMap f (WithTotalCount items _) =foldMap f items
instanceFromJSON (WithTotalCountArtifact) where
parseJSON = withObject "ArtifactList"$\o ->WithTotalCount<$> o .:"artifacts"<*> o .:"total_count
  • Tests. I have implemented tests for reading the artifact list. Downloading is more complex - it needs workflow permissions and does not work with an empty token. Even if https://github.com/phadej/github contained actions with artifacts, other users won't be able to run integration tests with download and deletion. Should I test only API with minimal required permissions?

Comment threadsrc/GitHub/Data/Actions.hs Outdated
}
deriving (Show, Data, Typeable, Eq, Ord, Generic)

data PaginatedWithTotalCount a (tag :: Symbol) = PaginatedWithTotalCount

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are these? Is artifacts pagination different then for everything else?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, please check my comment above. At least I didn't find existing implementation for this pagination style.

@K0Te
K0Te marked this pull request as draft July 16, 2022 23:58
@K0TeK0Te changed the title Implement actions->artifacts API.Implement Actions API.Jul 16, 2022
@K0Te

K0Te commented Jul 17, 2022

Copy link
Copy Markdown
ContributorAuthor

Hi @andreasabel ,

I got back to this issue and hope to implement full support for GitHub Actions.
Hope you could review this PR once it's ready and help to merge it.
There is one more interesting issue with the Actions API - https://docs.github.com/en/rest/actions/cache#delete-github-actions-caches-for-a-repository-using-a-cache-key defines a DELETE request with query parameters but at the moment only Queries support query parameters. Adding query parameters to command or Command would require changing lots of code, could there be a better approach?

@andreasabel

Copy link
Copy Markdown
Member

Hi @K0Te, good to have you back on track!

Hope you could review this PR once it's ready and help to merge it.

I'll do my best. I am not really an expert on this package.
Make sure your functionality is covered by tests or examples, remember to document, and adhere to the current style of the API, please.

There is one more interesting issue ... at the moment only Queries support query parameters.
Adding query parameters to command or Command would require changing lots of code, could there be a better approach?

I suppose you are referring to this code:

dataGenRequest (mt::MediaType*) (rw::RW) awhere
Query::Paths->QueryString->GenRequestmtrwa
PagedQuery:: (a~tb, Foldablet, Semigroupa) =>Paths->QueryString->FetchCount->GenRequestmtrwa
--| Command
Command
::CommandMethod--^ command
->Paths--^ path
->LBS.ByteString--^ body
->GenRequestmt 'RW a

query::Paths->QueryString->Requestmta
query ps qs =Query ps qs
pagedQuery::FromJSONa=>Paths->QueryString->FetchCount->Requestmt (Vectora)
pagedQuery ps qs fc =PagedQuery ps qs fc
command::CommandMethod->Paths->LBS.ByteString->Request 'RW a
command m ps body =Command m ps body

To keep API breakage at bay, I'd add it to Command but keep the smart constructor command as-is. Add a new smart constructor (e.g. paramCommand) that exposes the full feature set of Command.

@andreasabel

Copy link
Copy Markdown
Member

I resolved the conflicts with master so that CI can run.

@K0Te
K0Te marked this pull request as ready for review November 21, 2022 05:51
@K0Te

K0Te commented Nov 21, 2022

Copy link
Copy Markdown
ContributorAuthor

Hi @andreasabel ,

This PR is finally ready for review! 🚀
I have tested all endpoints added in the scope of this PR, everything seems to work fine on my test repo.
Some GitHub Actions API features are missing, so implementation does not cover all available API. However, I think that is should be enough for most use cases. Missing endpoints:

@K0Te

K0Te commented Apr 15, 2023

Copy link
Copy Markdown
ContributorAuthor

@andreasabel reminding about this PR ^_^

@andreasabelandreasabel left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! and sorry for the delayed review.

I had some stylistic requests but fixed them myself.

What remains is to document the WithTotalCount type.

Comment threadgithub.cabal Outdated
Comment threadspec/GitHub/Actions/ArtifactsSpec.hs
Comment threadsrc/GitHub/Data/Actions/Common.hs Outdated
Comment threadsrc/GitHub/Data/Actions/Cache.hs
Comment threadsrc/GitHub/Data/Actions/WorkflowJobs.hs Outdated
Comment threadsrc/GitHub/Data/Options.hs
Comment threadsrc/GitHub/Endpoints/Actions/Cache.hs Outdated
Comment threadsrc/GitHub/Data/Actions/Artifacts.hs Outdated
Comment threadsrc/GitHub/Data/Actions/Common.hs
Comment threadsrc/GitHub/Data/Actions/Common.hs
@andreasabelandreasabel added this to the 0.29 milestone Jun 23, 2023
@andreasabel

Copy link
Copy Markdown
Member

@andreasabelandreasabel left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed the last bits.

Good to go!

@andreasabel
andreasabel merged commit 5a26a8c into haskell-github:masterJun 24, 2023
@andreasabel

Copy link
Copy Markdown
Member

Published at https://hackage.haskell.org/package/github-0.29

Big thanks, @K0Te !

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add support for GitHub Actions

3 participants

@K0Te@andreasabel@phadej