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

yarn:update add a handled error for missing tags #8389

Merged
merged 1 commit into from
Nov 21, 2023

Conversation

pavera
Copy link
Contributor

@pavera pavera commented Nov 14, 2023

In the case of git dependencies, if the version is not tagged correctly the error is not caught as a resolvability error. This adds handling for the error message if a release is not tagged as expected.

@pavera pavera requested a review from a team as a code owner November 14, 2023 16:43
@jurre
Copy link
Member

jurre commented Nov 14, 2023

Any way to capture this in a test? Would that be valuable?

@pavera
Copy link
Contributor Author

pavera commented Nov 14, 2023

Any way to capture this in a test? Would that be valuable?

I'll take a look at existing tests and see what we have for other resolvability errors.

@@ -295,7 +295,8 @@ def handle_yarn_lock_updater_error(error, yarn_lock)
handle_timeout(error_message, yarn_lock) if error_message.match?(TIMEOUT_FETCHING_PACKAGE)

if error_message.start_with?("Couldn't find any versions") ||
error_message.include?(": Not found")
error_message.include?(": Not found") ||
Copy link
Member

Choose a reason for hiding this comment

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

minor: is this issue only specific to yarn or does it impact pnpm and npm too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is a specific yarn error message. There might be specific pnpm and npm error messages that are the "same" but they might already be matched by regex/strings in the respective code.

Copy link
Contributor

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

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

The way you explained this, it sounds like maybe Dependabot::GitDependencyReferenceNotFound is a better fit for this error?

Regarding testing this, my initial approach is normally:

  • Introduce a dummy change in a existing analogous regex/string.
  • Rerun tests and see what breaks.
  • Use the failing test as a starting point for a new test.

In the case of git dependencies, if the version is not tagged correctly the error is not caught as a resolvability error. This adds handling for the error message if a release is not tagged as expected.
@pavera pavera force-pushed the pavera/yarn_update_missing_tags branch from f2db961 to ef809af Compare November 21, 2023 20:46
@pavera pavera merged commit 77bd327 into main Nov 21, 2023
80 checks passed
@pavera pavera deleted the pavera/yarn_update_missing_tags branch November 21, 2023 21:09
@deivid-rodriguez
Copy link
Contributor

@pavera Any comments on my previous feedback?

@deivid-rodriguez
Copy link
Contributor

I'm good with merging this without tests if this is hard to test, but it sounds like the user error could be a better one if I understood your explanation correctly?

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.

4 participants