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

Example code: remove --no-interaction from Composer commands #547

Conversation

jrfnl
Copy link
Contributor

@jrfnl jrfnl commented Dec 26, 2021


name: ⚙ Improvement
about: You found a bug, want to improve something or add a new feature
labels: enhancement


Description

All Composer commands used in CI should use --no-interaction to prevent them hanging in case interaction is expected.

This adds the --no-interaction argument to all Composer example code which did not have the argument yet.

All Composer commands used in CI should use --no-interaction to prevent them hanging in case interaction is expected. However, as setupPHP sets the COMPOSER_NO_INTERACTION flag (as of this version), this is not needed when using setupPHP.

This removes redundant --no-interaction arguments from the example code.

Also see: https://blog.packagist.com/composer-2-2/#more-secure-plugin-execution

@shivammathur
Copy link
Owner

@jrfnl

Should setup-php set COMPOSER_NO_INTERACTION=1 instead of updating the examples?

// cc @Seldaek

@jrfnl
Copy link
Contributor Author

jrfnl commented Dec 26, 2021

Good question. I'm not sure.

There are couple of questions which come up in my mind thinking about it and I don't know the answers:

  • GH Actions related - if the env variable is set in a step, does it automatically get added to follow-on steps ? I vaguely recall that setting it on the workflow or job worked to propagate it to every step, but setting it in a step only made it available to that particular step. Not sure if that's still true and/or applies to predefined actions setting env variables.
  • Can the same env be set multiple times in a workflow/job ? I.e. does this behave like a constant or a variable ? I'm thinking about a few dedicated predefined actions, which may be used instead of manual calls to Composer, which may also be setting the env. Could setting the env from within this action cause conflicts/breakage ?
  • How does this behave if a particular command also sets the CLI argument ?

If needs be, I could go and do some investigating into this.

If setting COMPOSER_NO_INTERACTION=1 from this action would be a viable option, I do think it would need to be well documented in the Readme and possibly in more places to make people aware they don't need to add --no-interaction anymore.

@jrfnl
Copy link
Contributor Author

jrfnl commented Dec 26, 2021

Oh.. another question which comes to mind - when was support for this env added to Composer ? I.e. is it supported in Composer 1.x ?

@shivammathur
Copy link
Owner

There is a way to set it for all steps below using environment files.

If setup-php sets COMPOSER_NO_INTERACTION=1 and the workflow also sets this to 1 or passes --no-interaction in the composer step, there will be no error or warning about that, just that it would be redundant in the workflow.

Yes, it can be changed in later steps with the env property for the step.

Yes, documentation can be updated to reflect this if added.

Support for this was added in composer 1.0.0-alpha7. So it can be used without any issues with v1 as well.

@jrfnl
Copy link
Contributor Author

jrfnl commented Dec 26, 2021

In that case, I think setting the env will probably be a great improvement!

If you like, I can convert this PR to one removing the --no-interaction from the few commands in the examples which already passed the argument.

@shivammathur
Copy link
Owner

Yes, please do that. I have added COMPOSER_NO_INTERACTION=1 in e7e1eee in develop branch.

All Composer commands used in CI should use `--no-interaction` to prevent them hanging in case interaction is expected. However, as setupPHP sets the `COMPOSER_NO_INTERACTION` flag (as of this version), this is not needed when using setupPHP.

This removes redundant `--no-interaction` arguments from the example code.

Also see: https://blog.packagist.com/composer-2-2/#more-secure-plugin-execution
@jrfnl jrfnl changed the title Example code: use --no-interaction with Composer commands Example code: remove --no-interaction from Composer commands Dec 26, 2021
@jrfnl jrfnl force-pushed the feature/examples-composer-use-no-interaction branch from d744915 to 94aecb1 Compare December 26, 2021 19:44
@jrfnl
Copy link
Contributor Author

jrfnl commented Dec 26, 2021

Yes, please do that. I have added COMPOSER_NO_INTERACTION=1 in e7e1eee in develop branch.

Done! (including updating the PR title and description to match)

@shivammathur shivammathur merged commit 812d2bc into shivammathur:develop Dec 27, 2021
@jrfnl jrfnl deleted the feature/examples-composer-use-no-interaction branch December 27, 2021 00:18
@Seldaek
Copy link

Seldaek commented Feb 8, 2022

Eh well, now it turns out this change (well really e7e1eee) broke the Composer build ;)

https://github.com/composer/composer/runs/5109766175?check_suite_focus=true

The reason is the tests do not expect COMPOSER_PROCESS_TIMEOUT to be present in env. I guess I'll clean up the env before running the tests though as people may have this set locally as well. Probably no action to be taken here, just wanted to share because I found it somewhat funny that the tweak making Composer more reliable for others broke Composer's build.

@shivammathur
Copy link
Owner

@Seldaek
Sorry about this.
We previously also set process-timeout to 0, but that was with composer config command as it was the only configuration we set in most cases.

While adding more configurations it made sense to shift to an env file instead of calling composer for each of them.

@Seldaek
Copy link

Seldaek commented Feb 8, 2022

Really no worries, it makes complete sense here, and it's already fixed on my end, just confused me for a sec what changed. I didn't notice those env vars magically appeared in the middle of the run, I assumed I had set this myself :)

@jrfnl
Copy link
Contributor Author

jrfnl commented Feb 8, 2022

@Seldaek Oh cricky... of course it would... sorry about that. Then again "cleaning the environment" in the test bootstrap may actually be a good thing as I can imagine there may be contributors out there who also have ENV settings in place which could break the tests.
Then again - how to revert anything cleaned after the tests (for local test runs) ? Hmm.. maybe that could be done via a TestListener using the endTestSuite() method ? (with the clean-up in the startTestSuite() method and saving the original values - if any - to properties in the TestListener)

@Seldaek
Copy link

Seldaek commented Feb 8, 2022

Oh yeah this was absolutely a failure of the tests, and easily fixed composer/composer@3cb44bc

jrfnl added a commit to PHPCSStandards/PHPCSUtils that referenced this pull request Mar 26, 2022
... as since `setup-php` version `2.17.0`, this is no longer needed as this is now the default via an environtment variable set by `setup-php`.

Refs:
* https://github.com/shivammathur/setup-php/releases/tag/2.17.0
* shivammathur/setup-php#547
@jrfnl jrfnl mentioned this pull request Aug 20, 2022
1 task
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.

None yet

3 participants