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

feat: search matching releases on GitHub before falling back to tags #169

Merged
merged 1 commit into from
Jun 27, 2022

Conversation

maribethb
Copy link
Contributor

@maribethb maribethb commented Jun 24, 2022

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Did not file an issue as discussed with Ben offline.

This PR adds additional checking for release-backed tokens. Release-backed tokens require a matching tag on GitHub. Previously this worked by fetching 11 pages of tags from GitHub and searching for a match. This approach fails for large monorepos, as each release of each package in the monorepo has its own tag. The GitHub API does not return tags in reverse chronological order. So this approach only worked for repos with under 1100 tags.

This change first checks GitHub for a matching release, because the API allows you to ask for a particular release by tag name. If a matching release is not found, it falls back to searching all tags. This is for those projects that are using tags but not GitHub releases.

Updated tests to cover both cases. Updated jsdoc for relevant methods.

Also, if there is an error while fetching releases/tags, I added the error text to the resulting message to increase debuggability.

Note: I tested this with unit tests but I wasn't able to test this with an actual running server, because I don't have all the relevant config. This should probably be tested with a running server since the unit tests can't account for e.g. misspelling the API or something. I did test the API calls with curl on the command line, but still could have made an error in the code.

Copy link
Contributor

@bcoe bcoe left a comment

Choose a reason for hiding this comment

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

:shipit:

@bcoe bcoe merged commit 8deacf2 into GoogleCloudPlatform:main Jun 27, 2022
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.

2 participants