Skip to content

Implement Actions API. #459

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 38 commits into from
Jun 24, 2023
Merged

Conversation

K0Te
Copy link
Contributor

@K0Te K0Te commented Jun 22, 2021

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:
data ArtifactList = ArtifactList
    { artifactListArtifacts :: !(Vector Artifact)
    , 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:
data PaginatedWithTotalCount a (tag :: Symbol) = PaginatedWithTotalCount
    { paginatedWithTotalCountItems :: !(Vector a)
    , 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...
data WithTotalCount a = WithTotalCount
    { withTotalCountItems :: !(Vector a)
    , withTotalCountTotalCount :: !Int
    } deriving (Show, Data, Typeable, Eq, Ord, Generic)

instance Semigroup (WithTotalCount a) where
    (WithTotalCount items1 count1) <> (WithTotalCount items2 _) =
        WithTotalCount (items1 <> items2) count1

instance Foldable WithTotalCount where
    foldMap f (WithTotalCount items _) = foldMap f items

instance FromJSON (WithTotalCount Artifact) 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?

}
deriving (Show, Data, Typeable, Eq, Ord, Generic)

data PaginatedWithTotalCount a (tag :: Symbol) = PaginatedWithTotalCount
Copy link
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
Contributor Author

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
@K0Te K0Te changed the title Implement actions->artifacts API. Implement Actions API. Jul 16, 2022
@K0Te
Copy link
Contributor Author

K0Te commented Jul 17, 2022

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
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:

data GenRequest (mt :: MediaType *) (rw :: RW) a where
Query :: Paths -> QueryString -> GenRequest mt rw a
PagedQuery :: (a ~ t b, Foldable t, Semigroup a) => Paths -> QueryString -> FetchCount -> GenRequest mt rw a
-- | Command
Command
:: CommandMethod -- ^ command
-> Paths -- ^ path
-> LBS.ByteString -- ^ body
-> GenRequest mt 'RW a

query :: Paths -> QueryString -> Request mt a
query ps qs = Query ps qs
pagedQuery :: FromJSON a => Paths -> QueryString -> FetchCount -> Request mt (Vector a)
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
Member

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

@andreasabel andreasabel self-assigned this Nov 12, 2022
@K0Te K0Te marked this pull request as ready for review November 21, 2022 05:51
@K0Te
Copy link
Contributor Author

K0Te commented Nov 21, 2022

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
Copy link
Contributor Author

K0Te commented Apr 15, 2023

@andreasabel reminding about this PR ^_^

Copy link
Member

@andreasabel andreasabel left a comment

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.

@andreasabel andreasabel added this to the 0.29 milestone Jun 23, 2023
@andreasabel
Copy link
Member

Copy link
Member

@andreasabel andreasabel left a comment

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:master Jun 24, 2023
@andreasabel
Copy link
Member

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

Big thanks, @K0Te !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for GitHub Actions
3 participants