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

Support using custom PHP environment variables to pass API key to paid plugins #2001

Closed
Gandhi11 opened this issue Apr 2, 2020 · 5 comments
Labels
F: private-registries 💂‍♂️ Issues about using private registries with Dependabot; may be paired with an R: label. L: php:composer Issues and code for Composer T: feature-request Requests for new features

Comments

@Gandhi11
Copy link

Gandhi11 commented Apr 2, 2020

Hi,

I'm using Bedrock with wordpress to be able to install my plugins using composer. For premium plugins, I am using private-composer-installer to install them. I am injecting multiple environment variables which are the keys to download the plugins.

Currently there are only two variables supported: ACF_PRO_KEY and GRAVITY_FORMS_KEY. Is there any way to add other ones?

For example, I am using these for ACF Pro and WPML.

  • PLUGIN_ACF_KEY (I could change this one to ACF_PRO_KEY)
  • PLUGIN_WPML_SUBSCRIPTION_KEY
  • PLUGIN_WPML_USER_ID
@infin8x infin8x transferred this issue from dependabot/feedback Jun 29, 2020
@viktorbijlenga
Copy link

We also use Bedrock by Roots to manage WordPress. We then use Dependabot to manage our WordPress plugins, with great help by the excellent WordPress Packagist.

Currently our main use of PHP environment variables is to manage the plugin Advanced Custom Fields Pro, which depends on a PHP environment variable to be updated as explained by @Gandhi11 above.

Our composer solution for ACF is using PiventIT/ACF-pro-installer by @Qrious. A lot of people is also using PhilippBaschke /acf-pro-installer. On top of using ACF this way, we also have other plugins which depends on a similar workflow.

A week ago we started see a new pull reuqest from Dependabot across our repos, with information regarding the future of Dependabot. The information has the following part which is related to custom PHP enciroment variables in the future.

Your repo is using PHP environment variables. There is no support for this registry type in GitHub-native Dependabot, so these registries were not added to the new config file. We recommend fetching these dependencies via GitHub Actions.

I would love to see a future in which it's possible to still update dependencies that need a PHP env variable in a good way. Anyone who have ideas on a good way forward that would work for a multitude of plugins (maybe using Github Actions as mentioned) would be a great addition to the PHP/WordPress community.

I would love if @asciimike or someone from the Dependabot team would pitch in here, since it would be great to hear their ideas in more detail, on how those who depends on this, could move this forward together.

@asciimike
Copy link
Contributor

The current thought for folks doing this is: run composer update in an Action (using an on: pull_request_target from a Dependabot PR as the trigger) and provide the ACF_PRO secret in Actions secrets (why you have to use pull_request_target). It's possible you may have to remove the ACF dependency in your composer.json for normal runs (if it's causing a failure that isn't updating other dependencies), and keep around a composer-dependabot.json that has that additional dependency (or only contains it) and swap it during that run.

@Qrious
Copy link

Qrious commented May 12, 2021

This situation is undesired in my opinion, certainly because it is a significant step backwards compared to the preview.

It's possible you may have to remove the ACF dependency in your composer.json for normal runs (if it's causing a failure that isn't updating other dependencies), and keep around a composer-dependabot.json

My main issue with this 'solution' is that it complicates the flow of all developers in the teams that need to understand this workaround and remember it at all times when interacting with composer. It is very easy to do it wrong, since a normal composer install appears to work just fine, but you end up missing the packages that need PHP environment variables for installation (i.e. ACF). Secondly users need to keep both composer.json files in sync, which is an additional burden.

@viktorbijlenga
Copy link

viktorbijlenga commented May 12, 2021

A way forward which would require to manage multiple composer-files to update a few dependencies (in our case) is not a solution that feels great. It would add more work, to something which is aimed at creating less work. We would probably then start ignoring these dependecies from Dependabot. It feels like a step backwards.

I would propose that the environment-variable support is added to the native version, since it's working in Dependabot Preview. It would make Dependabot more useful for all of us that need to work with environment variables.

I imagiae that the syntax part could look something like this:

version: 2
updates:
  - package-ecosystem: composer
    directory: "/"
    schedule:
      interval: weekly
    environment-variable:
      - ACF_PRO_KEY: ${{secrets.MY_ACF_PRO_KEY}}

@jurre jurre added L: php:composer Issues and code for Composer T: feature-request Requests for new features labels Nov 30, 2021
@jeffwidman jeffwidman changed the title Custom PHP environment variables Support using custom PHP environment variables to pass API key to paid plugins Feb 10, 2023
@jeffwidman jeffwidman added the F: private-registries 💂‍♂️ Issues about using private registries with Dependabot; may be paired with an R: label. label Feb 10, 2023
@jeffwidman
Copy link
Member

Since this was posted, the PHP packaging world has continued to mature and standardize on composer, which includes support for private registries.

For example, the ACF_PRO_KEY env var mentioned above is no longer needed because ACF Pro recently rolled out their own private registry:

Closing as won't fix because we aren't going to add support for custom environment vars for authentication / secrets purposes. Instead, ask your plugin provider to offer their own private registry... not only will it work out of the box with Dependabot, it'll also work much better with the rest of the PHP/composer tooling ecosystem.

@jeffwidman jeffwidman closed this as not planned Won't fix, can't repro, duplicate, stale Feb 10, 2023
jeffwidman added a commit that referenced this issue Jul 31, 2023
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F: private-registries 💂‍♂️ Issues about using private registries with Dependabot; may be paired with an R: label. L: php:composer Issues and code for Composer T: feature-request Requests for new features
Projects
None yet
Development

No branches or pull requests

6 participants