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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dont mutate env vars outside the BuildEnv classes #8340

Merged
merged 2 commits into from Jul 13, 2021
Merged

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Jul 13, 2021

We are mutating this variable on the environment instance, and that's being passed to the next commands.

self.environment.environment.update(self.env)

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.

class BuildEnv:

    @property
    def environment(self):
        return self._environment

Fixes #8288
Closes #8318

@stsewd stsewd marked this pull request as draft July 13, 2021 02:08
@stsewd stsewd marked this pull request as ready for review July 13, 2021 14:56
@stsewd stsewd requested a review from a team July 13, 2021 15:04
@@ -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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

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

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()
Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see we're still passing them here 馃憤

@stsewd stsewd added the PR: hotfix Pull request applied as hotfix to release label Jul 13, 2021
@stsewd stsewd merged commit 7e8f6c7 into master Jul 13, 2021
@stsewd stsewd deleted the dont-mutate-env-vars branch July 13, 2021 21:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: hotfix Pull request applied as hotfix to release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom LFS integration has broken unexpectedly
2 participants