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

fix: respond to 404 errors when checking releases, and fall back to tags #171

Merged
merged 1 commit into from
Jul 6, 2022

Conversation

maribethb
Copy link
Contributor

  • 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)

Fixes #158

  1. Adds a try/catch around the entire check to getReleaseForTags. If this fails for any reason then we will fall back to checking the tags.
  2. Handles the 404 case for a release cleanly. The previous code was wrong because I assumed that the API used would return a code 404 and an empty response if the release was missing, similar to what happens if you call the GitHub API directly with curl. However, this client library we're using (which is not the standard one) actually returns an error object for this case. Unfortunately I could not find any definitions for the type of Error object they're using, but after trial and error we determined the error does have a property called statusCode that we can check. If it's 404 then we just continue to the next tag, if it's another error then we'll throw the error, but it will be caught by step 1 and we'll fall back to tags.
  3. Updated all of the tests to use replyWithError to more closely match how the API behaves in reality.
  4. Added a test for the case where we get a non-404 error from the getReleaseForTags call.
  5. Fixed a problem with the error messages in write-message. If this method catches an error that isn't a WombatServerError then it would just say "unknown error." Now, it can handle either WombatServerError or just regular Error (which has a message property not a statusMessage property) but still returns an error with the same shape (before, it was just an object that had the same properties as WombatSeverError, now it will be an actual instance of it). I tested this change by causing a generic error by not providing all of the mocked API responses to nock in a test. Previously, the error message would be a cryptic "unknown error" and now the error actually contains the useful message from nock. This isn't directly related to the core issue I was facing but it made it easier to debug after I fixed this.

I tested this change by actually using this GitHub client library in a separate test program and observing it worked the way I wanted it to. See this gist which I invoked using node index.js which actually performs the GH API call (instead of relying on unit tests with mocked API responses). So I am reasonably confident this change works as intended now.

err.statusCode = 500;
err.statusMessage = 'unknown error';
throw err;
} catch (err) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm glad that you addressed this TODO while you were at it.

@bcoe bcoe merged commit 89746a5 into GoogleCloudPlatform:main Jul 6, 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.

Swallowed error if there are fewer than 11 pages of tags
2 participants