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

feature: add update-environment input #411

Merged
merged 1 commit into from Jun 29, 2022
Merged

Conversation

mayeut
Copy link
Contributor

@mayeut mayeut commented May 26, 2022

Description:
This option allows to specify if the action shall update environment variables (default) or not.
This allows to use the setup-python action in a composite action without side effect (except downloading/installing python if version is missing).

Related issue:
fix #410

Check list:

  • Mark if documentation changes are required.
  • Mark if tests were added or updated to cover the changes.

@mayeut mayeut requested a review from a team as a code owner May 26, 2022
@@ -24,7 +24,7 @@
"license": "MIT",
"dependencies": {
"@actions/cache": "^2.0.2",
"@actions/core": "^1.2.3",
"@actions/core": "^1.7.0",
Copy link
Contributor Author

@mayeut mayeut May 26, 2022

Choose a reason for hiding this comment

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

This required to be bumped to >= 1.3.0 to get getBooleanInput support. I bumped to the same version as what's being proposed in #406

@mayeut
Copy link
Contributor Author

mayeut commented May 26, 2022

e2e-cache / Test poetry (Python pypy-3.7-v7.x, windows-latest) failure has nothing to do with this PR.

Copy link
Contributor

@brcrista brcrista left a comment

I'd like to explore other solutions besides adding complexity to the action.

Can you just add steps to the composite action? For example:

# Save the environment
set > env.sh

# Restore the environment
source env.sh

@mayeut
Copy link
Contributor Author

mayeut commented May 26, 2022

@brcrista,
there's no way to revert a core.addPath or a core.exportVariable

For exportVariable:
If they were already set, then the ones with exportVariable could be overriden back to their original values.
If they were not set, it's impossible to unset them: https://stackoverflow.com/questions/70137245/how-to-remove-an-environment-variable-on-github-actions

For addPath, it's values are always prepended to the "current" PATH, c.f. actions/toolkit#655

@vsafonkin
Copy link
Contributor

vsafonkin commented Jun 1, 2022

Hi @mayeut, I have some concerns about this feature. In order to use python that is installed by action without $PATH updating you should use exact paths to the installed python in a composite action. If there are any other tools in composite action which use python implicitly, then these tools will not use the version you expect. This can lead to confusion about which python version is used in some command or tool.

From my point of view an installation of python without adding to PATH may create more complications than solve problems.

@mayeut
Copy link
Contributor Author

mayeut commented Jun 4, 2022

In order to use python that is installed by action without $PATH updating you should use exact paths to the installed python in a composite action. If there are any other tools in composite action which use python implicitly, then these tools will not use the version you expect. This can lead to confusion about which python version is used in some command or tool.
From my point of view an installation of python without adding to PATH may create more complications than solve problems.

@vsafonkin,

This is an opt-in feature. It won't change a thing for existing users. If they choose to opt-in, they shall know the implications.
Furthermore, you can easily test your own action with a GHA workflow to check if it's working as expected or not but you can't possibly test how you'll break hundreds of downstream workflows using your action if you mess up the environment. I'd rather just get ahead of this most likely event by not messing with the environment in the first place.
Last time setup-python introduced a change in environment variables, it did create some issues (#159). I'd rather avoid that.

As said in an earlier comment, it's just impossible to restore the environment as it was before running setup-python. Even if it were, I feel that it would be better solved by adding a bit of complexity in this action, centralized, rather than incurring workarounds for downstream users.

@vsafonkin
Copy link
Contributor

vsafonkin commented Jun 6, 2022

@mayeut, ok, I see. Since the AzDev UsePython task has a similar input addToPath I think it makes sense to add this feature to the action.

Copy link
Contributor

@brcrista brcrista left a comment

I'll defer to @vsafonkin on this

@brcrista brcrista requested review from brcrista and vsafonkin Jun 23, 2022
action.yml Outdated
no-environment-update:
description: 'Set this option if you want the action not to update environment variables.'
default: false
Copy link
Contributor

@brcrista brcrista Jun 23, 2022

Choose a reason for hiding this comment

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

Suggested change
no-environment-update:
description: 'Set this option if you want the action not to update environment variables.'
default: false
update-environment:
description: 'Set this option if you want the action to update environment variables.'
default: true

Putting boolean inputs in positive terms is usually easier for people to understand.

Copy link
Contributor Author

@mayeut mayeut Jun 23, 2022

Choose a reason for hiding this comment

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

done

This option allows to specify if the action shall update environment variables (default) or not.
This allows to use the setup-python action in a composite action without side effect (except downloading/installing python if version is missing).
@mayeut mayeut changed the title feature: add no-environment-update input feature: add update-environment input Jun 23, 2022
@brcrista brcrista merged commit 00a5248 into actions:main Jun 29, 2022
147 of 151 checks passed
@mayeut mayeut deleted the no-envvar branch Jun 29, 2022
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.

feature: add an option not to add/update environment variables
6 participants