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

Stop explicitly catching missing env-var error in composer #7675

Closed
wants to merge 1 commit into from

Conversation

jeffwidman
Copy link
Member

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: Dependabot environment variables #4660
  2. The primary use case was for ACF Pro, but that is no longer relevant since they now support private composer auth natively: Support using custom PHP environment variables to pass API key to paid plugins #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: Expose composer error message when not resolvable #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.

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.
@jeffwidman jeffwidman requested a review from a team as a code owner July 31, 2023 20:08
@github-actions github-actions bot added the L: php:composer Issues and code for Composer label Jul 31, 2023
@deivid-rodriguez
Copy link
Contributor

deivid-rodriguez commented Nov 21, 2023

Turns out I need this error for other ecosystems. For example, this repo includes a .yarnrc.yml file that uses an environment variable, and that prevents dependabot from running. This user error is perfect for explaining this, so I'm closing this and will be opening a PR to catch this Yarn error and raise it as Dependabot::MissingEnvironmentVariable.

Let me know if you disagree though!

@deivid-rodriguez deivid-rodriguez deleted the stop-explicitly-catching-env-var-error branch November 21, 2023 20:12
@jeffwidman
Copy link
Member Author

Makes sense to me to keep it. 👍

@deivid-rodriguez
Copy link
Contributor

#8446 will start using this error class 👍.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L: php:composer Issues and code for Composer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants