Dont mutate env vars outside the BuildEnv classes #8340
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| @@ -370,7 +377,7 @@ def run(self, version_pk): # pylint: disable=arguments-differ | |||
| version=self.version, | |||
| record=False, | |||
| update_on_success=False, | |||
| environment=self.get_rtd_env_vars(), | |||
| environment=self.get_vcs_env_vars(), | |||
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
I'd also like to see get_env_vars renamed to get_build_env_vars if we're doing this refactor, to keep things clear.
ericholscher
approved these changes
Jul 13, 2021
There was a problem hiding this comment.
This is a better approach. The mixing of different ways to set the env is definitely not good. We're already handling a lot of env setting in projects/tasks.py, so this makes sense to me. I'm not really sure how we ended up with env on the VCS commands as well.
| @@ -311,6 +311,13 @@ def validate_duplicate_reserved_versions(self, tags_data, branches_data): | |||
| RepositoryError.DUPLICATED_RESERVED_VERSIONS, | |||
| ) | |||
|
|
|||
| def get_vcs_env_vars(self): | |||
| """Get environment variables to be included in the VCS setup step.""" | |||
| env = self.get_rtd_env_vars() | |||
There was a problem hiding this comment.
Oh, I see we're still passing them here 👍
stsewd
force-pushed
the
dont-mutate-env-vars
branch
from
4e1c4ec to
5cf3350
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We are mutating this variable on the environment instance, and that's being passed to the next commands.
readthedocs.org/readthedocs/vcs_support/base.py
Line 76 in babbdf0
Previous to #8263 we were creating a new instance (LocalBuildEnv) so this bug wasn't visible, but now we are using the same env to be able to always execute these commands inside docker (except for gitpython that still runs outside the container).
Now I have moved the env var to avoid the prompt outside git (GIT_DIR isn't needed as explained in #8318 (review)) and I made environment private so isn't mutated outside the class without thinking twice (because we would never mutate a private property, right? 👀).
We could also maybe expose this as a property, so is read-only, but we only use this on a test.
Fixes #8288
Closes #8318