Skip to content
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

[GitHub] add latest release of a branch badge #9661

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

karolwydmuch
Copy link

No description provided.

@github-actions
Copy link
Contributor

Messages
📖 ✨ Thanks for your contribution to Shields, @karolwydmuch!

Generated by 🚫 dangerJS against a63bc37

@chris48s
Copy link
Member

Hi. Thanks for opening a PR.

A badge should be quick to render. In most contexts where our badges are displayed there's a hard limit of 4 seconds on a request. In general we try usually avoid accepting badges that require more than one network call to render for this reason.

This means badges that require iterating over multiple pages of API response are impractical. They end up being too slow to render in a lot of cases.

I had a look to see if it would be possible to obtain this information in a single GraphQL query but it seems this is not exposed by the GraphQL API. https://docs.github.com/en/graphql/reference/objects#release

If we can't find a more efficient way to query it, I'm inclined to decline this one.

Out of interest, and also for testing purposes, do you have an example of a repo that publishes releases against different branches? It is not something I have come across before and I don't think this corresponds to an open feature request.

})

t.create('Release')
.get('/v/release/expressjs/express.json')
Copy link
Member

Choose a reason for hiding this comment

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

If we end up coming back to this, none of the tests in this file are actually testing the service class that has been added in this PR, but lets not get bogged down in that for the moment

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

Successfully merging this pull request may close these issues.

None yet

3 participants