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

Handle DependencyFileNotParseable error on Updater.run #11288

Merged
merged 5 commits into from
Jan 16, 2025

Conversation

amazimbe
Copy link
Contributor

@amazimbe amazimbe commented Jan 13, 2025

What are you trying to accomplish?

Handle a DependencyFileNotParseable error when running the Updater so that this error doesn't get propagated up the chain as an unknown error. It also won't appear in Sentry once it's been handled correctly.

We are already handling the exception on line https://github.com/dependabot/dependabot-core/blob/main/updater/lib/dependabot/update_files_command.rb#L27 and this just reapplies the same logic.

How will you know you've accomplished your goal?

The Sentry issue specified above should cease to appear.

Checklist

  • I have run the complete test suite to ensure all tests and linters pass.
  • I have thoroughly tested my code changes to ensure they work as expected, including adding additional tests for new functionality.
  • I have written clear and descriptive commit messages.
  • I have provided a detailed description of the changes in the pull request, including the problem it addresses, how it fixes the problem, and any relevant details about the implementation.
  • I have ensured that the code is well-documented and easy to understand.

@amazimbe amazimbe force-pushed the amazimbe/fix-unhandled-composer-exceptions branch from 1ad0c6a to 94d2bed Compare January 13, 2025 15:09
@amazimbe amazimbe changed the title Bump php to 8.2 (#6506) Handle DependencyFileNotParseable error on Updater.run Jan 13, 2025
@amazimbe amazimbe force-pushed the amazimbe/fix-unhandled-composer-exceptions branch from 94d2bed to 1906bda Compare January 13, 2025 15:28
@amazimbe amazimbe marked this pull request as ready for review January 13, 2025 15:29
@amazimbe amazimbe requested a review from a team as a code owner January 13, 2025 15:29
@amazimbe amazimbe self-assigned this Jan 13, 2025
markhallen
markhallen previously approved these changes Jan 13, 2025
@amazimbe amazimbe force-pushed the amazimbe/fix-unhandled-composer-exceptions branch from 1906bda to 3b8bd40 Compare January 13, 2025 17:30
@amazimbe amazimbe force-pushed the amazimbe/fix-unhandled-composer-exceptions branch 3 times, most recently from d921dfd to f226cef Compare January 14, 2025 14:08
@amazimbe amazimbe requested a review from jakecoffman January 14, 2025 14:39
jakecoffman
jakecoffman previously approved these changes Jan 14, 2025
Copy link
Member

@jakecoffman jakecoffman left a comment

Choose a reason for hiding this comment

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

LGTM, I would update the test assertion as I mentioned above but approving in the meantime.

@amazimbe amazimbe force-pushed the amazimbe/fix-unhandled-composer-exceptions branch 3 times, most recently from 6f84548 to 94e1003 Compare January 15, 2025 15:20
return unless Experiments.enabled?(:record_update_job_unknown_error)

service.record_update_job_unknown_error(
error_type: T.must(error_details).fetch(:"error-type"),
Copy link
Member

Choose a reason for hiding this comment

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

any reason to record this as an unknown error again?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mostly because that's what's being done with the same error in this same file a few lines earlier. I'm not completely sure what the rules are for this categorisation so I just went with the precedent.

@amazimbe amazimbe force-pushed the amazimbe/fix-unhandled-composer-exceptions branch from 94e1003 to 1a21d40 Compare January 15, 2025 15:32
@amazimbe amazimbe force-pushed the amazimbe/fix-unhandled-composer-exceptions branch from 1a21d40 to 5038fd5 Compare January 16, 2025 09:17
@amazimbe amazimbe merged commit 43e0298 into main Jan 16, 2025
128 checks passed
@amazimbe amazimbe deleted the amazimbe/fix-unhandled-composer-exceptions branch January 16, 2025 09:54
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.

5 participants