FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Implement Actions API. by K0Te · Pull Request #459 · haskell-github/github · GitHub

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

Comment thread src/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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

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

K0Te commented Jul 17, 2022

Copy link
Copy Markdown
Contributor Author

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?

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:

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.

Copy link
Copy Markdown
Member

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

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

K0Te commented Nov 21, 2022

Copy link
Copy Markdown
Contributor Author

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 commented Apr 15, 2023

Copy link
Copy Markdown
Contributor Author

@andreasabel reminding about this PR ^_^

andreasabel 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 thread github.cabal Outdated
Comment thread src/GitHub/Data/Actions/Common.hs Outdated
Comment thread src/GitHub/Data/Actions/WorkflowJobs.hs Outdated
Comment thread src/GitHub/Endpoints/Actions/Cache.hs Outdated
Comment thread src/GitHub/Data/Actions/Artifacts.hs Outdated
andreasabel added this to the 0.29 milestone Jun 23, 2023

Copy link
Copy Markdown
Member

Candidate for 0.29 at: https://hackage.haskell.org/package/github-0.29/candidate

andreasabel 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Addressed the last bits.

Good to go!

andreasabel merged commit 5a26a8c into haskell-github:master Jun 24, 2023

Copy link
Copy Markdown
Member

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

Big thanks, @K0Te !

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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


Back | FazBrowse Home | New Git URL