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.

Sign up for GitHub

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

Jump to bottom

Fix Corepack install failure by activating local package manager fallback #11484

Merged
merged 2 commits into from
Feb 4, 2025
Merged

Fix Corepack install failure by activating local package manager fallback #11484

merged 2 commits into from
Feb 4, 2025

Conversation

kbukum1
Copy link
Contributor

@kbukum1 kbukum1 commented Feb 4, 2025 •
edited
Loading

What are you trying to accomplish?

The fetch_files phase was failing due to a Corepack installation issue with pnpm. The error stemmed from an inability to verify the package manager's signature when installing [email protected]. This caused the entire process to fail.

This PR addresses the issue by activating the local package manager as a fallback when Corepack installation fails. This ensures that Dependabot can proceed with file fetching even if Corepack encounters verification issues.

Anything you want to highlight for special attention from reviewers?

  • The fallback mechanism was chosen to prevent hard failures during pnpm installation and instead allow Dependabot to use an already available version.
  • If there are concerns about security implications, we can discuss alternative solutions, such as stricter validation checks before activating the fallback.

How will you know you've accomplished your goal?

  • Dependabot no longer fails during the fetch_files phase due to pnpm installation issues.
  • Running bin/run fetch_files now completes successfully without encountering file_fetcher_error.
  • Local testing confirms that the fallback mechanism works correctly when Corepack installation fails.

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.

@kbukum1 kbukum1 marked this pull request as ready for review February 4, 2025 21:40
@kbukum1 kbukum1 requested a review from a team as a code owner February 4, 2025 21:40
kbukum1
kbukum1 commented Feb 4, 2025
View reviewed changes
npm_and_yarn/lib/dependabot/npm_and_yarn/package_manager.rb
rescue SharedHelpers::HelperSubprocessFailed => e
Dependabot.logger.error("Error installing #{name}@#{version}: #{e.message}")
Helpers.fallback_to_local_version(name)
end
Copy link
Contributor Author

@kbukum1 kbukum1 Feb 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Review Tip: Implement a fallback mechanism to activate the local package manager if corepack installation fails.

@kbukum1 kbukum1 marked this pull request as draft February 4, 2025 21:55
@kbukum1 kbukum1 force-pushed the kamil/fix_corepack_install_failing_issue branch from 5baf649 to c197736 Compare February 4, 2025 22:32
@kbukum1 kbukum1 marked this pull request as ready for review February 4, 2025 22:34
sachin-sandhu
sachin-sandhu approved these changes Feb 4, 2025
View reviewed changes
Copy link
Contributor

@sachin-sandhu sachin-sandhu left a comment

Choose a reason for hiding this comment

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

LGTM.

abdulapopoola
abdulapopoola reviewed Feb 4, 2025
View reviewed changes
npm_and_yarn/lib/dependabot/npm_and_yarn/helpers.rb
@@ -467,6 +467,8 @@ def self.install(name, version, env: {})
# Attempt to activate the local version of the package manager
sig { params(name: String).void }
def self.fallback_to_local_version(name)
return "Corepack does not support #{name}" unless corepack_supported_package_manager?(name)

Dependabot.logger.info("Falling back to activate the currently installed version of #{name}.")
Copy link
Member

Choose a reason for hiding this comment

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

is it possible to get test coverage for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apologies, I just saw your message.

We have spec for fallback case shared in the reference link. Regarding the bun condition, I added it as a precaution in case it gets called. Since @markhallen checks for other commands, I wanted to ensure it doesn’t unintentionally affect bun.

If needed, I can create another PR to add specs for:

return "Corepack does not support #{name}" unless corepack_supported_package_manager?(name)

Let me know if you think this is necessary.

Reference: [helpers_spec.rb#L314](https://github.com/dependabot/dependabot-core/blob/main/npm_and_yarn/spec/dependabot/npm_and_yarn/helpers_spec.rb#L314)

@kbukum1 kbukum1 merged commit 3508888 into main Feb 4, 2025
88 checks passed
@kbukum1 kbukum1 deleted the kamil/fix_corepack_install_failing_issue branch February 4, 2025 23:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abdulapopoola abdulapopoola abdulapopoola left review comments

@sachin-sandhu sachin-sandhu sachin-sandhu approved these changes

Assignees
No one assigned
Labels
L: javascript
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kbukum1 @abdulapopoola @sachin-sandhu