-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Expose composer error message when not resolvable #6290
base: main
Are you sure you want to change the base?
Conversation
36c1763
to
6adc96e
Compare
Putting this on ice for a bit to brainstorm possible ways to handle when missing PHP native extensions as mentioned in #6289 (comment) |
Expose the underlying composer error message when a dependency is not resolvable. While debugging a customer issue, we ran into the problem that the error output from the `composer` helper was being swallowed: https://github.com/dependabot/dependabot-core/blob/b911f9dfe1e11d19edcafb11e83d305758c97f85/composer/lib/dependabot/composer/update_checker/version_resolver.rb#L295-L304 The underlying error: ``` Your requirements could not be resolved to an installable set of packages. (Dependabot::SharedHelpers::HelperSubprocessFailed) Problem 1 - php-amqplib/php-amqplib v2.11.3 conflicts with php[7.4.0]. - php-amqplib/php-amqplib v2.11.3 conflicts with php[7.4.0]. - Installation request for php 7.4.0 -> satisfiable by php[7.4.0]. - Installation request for php-amqplib/php-amqplib (locked at v2.11.3, required as ^2.6) -> satisfiable by php-amqplib/php-amqplib[v2.11.3]. ``` Swallowing the error makes it hard for users to figure what's going on. The original motivation for swallowing was in the early days of Dependabot Preview, it opened issues for exceptions raised in the updater/core which has forced us to suppress certain errors that might be spammy or intermittent, e.g. resolvability errors. In version updates we no longer create issues so attaching errors to the last update job means we can attach any number of errors to highlight potential issues without spamming users. Fix #6289
6adc96e
to
e7578ea
Compare
We can probably switch this to logging the error message, so it's still visible in the job logs, but doesn't actually fail the job due to the problem mentioned in #6289 (comment) |
This removes explicitly catching errors about missing env vars. This is likely a bit controversial, so here's my thinking: 1. Dependabot at GitHub doesn't support injecting custom env vars. And for security reasons is unlikey to anytime soon: #4660 2. The primary use case was for `ACF Pro`, but that is no longer relevant since they now support private `composer` auth natively: #2001 (comment) 3. In general we've told users that if they need custom env vars for authentication reasons, they should instead ask their upstream providers to use the standard `composer` tooling, rather than a workaround. We aren't going to go out of our way to support non-standard tooling practices. 4. Setting env vars is still relevant for anyone who runs Dependabot standalone, but those are typically power users who are very comfortable debugging / reading the logs to realize the error is the lack of an env var. 5. Technically this could cause more internal alerts because removing this error changes those updates from being seen as "well-known user errors" to "unknown service errors" (which we internally alert on). However, I strongly suspect that very few users actually hit this problem... so if just a couple of repos are hitting it, that shouldn't be unduly affecting our metrics. 6. Currently unknown `composer` errors are hidden, but we should resolve that with a systemic solution rather than one-off-solutions for specific packages, which turns into a game of whack-a-mole: #6290 7. Composer error handling is a big mess in our code... it's copy/pasted several places, with `TODO` notes saying we should refactor/cleanup our error handling code. And we don't even know what's still relevant once we drop `composer` `1` as the error messages changed a bit. So any cleanup / simplification we can do of the error handling code makes our future refactoring easier. If we needed this generic error for other ecosystems, that'd make sense, but so far it's `composer` specific so until we need that, let's just remove it.
Uh, thanks for the review/approve, but we don't want to merge this as-is... see the comment thread above. |
I have opened a new PR to resolve this, so I am closing this PR: #9657 |
That's awesome you're tackling it! However, let's not actually close PR's until another PR is actually merged... because my experience is not all PR's get merged, and then we end up with a world of forgetting to re-open an original PR that may be halfway done. ☝️ is a general best practice that I see followed in most open source repos, not specific just to this PR. |
Expose the underlying composer error message when a dependency is not resolvable.
While debugging a customer issue, we ran into the problem that the error output from the
composer
helper was being swallowed:dependabot-core/composer/lib/dependabot/composer/update_checker/version_resolver.rb
Lines 295 to 304 in b911f9d
The underlying error:
Swallowing the error makes it hard for users to figure what's going on.
The original motivation for swallowing was in the early days of Dependabot Preview, it opened issues for exceptions raised in the updater/core which has forced us to suppress certain errors that might be spammy or intermittent, e.g. resolvability errors. In version updates we no longer create issues so attaching errors to the last update job means we can attach any number of errors to highlight potential issues without spamming users.
Fix #6289