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

Be explicit when using setup_env #6451

Merged
merged 3 commits into from Dec 17, 2019
Merged

Conversation

@stsewd
Copy link
Member

stsewd commented Dec 10, 2019

Currently, when sync_repository_task gets triggered we use a LocalEnvironment as environment.

We want to be explicit about respecting the docker setting in all places where we do a clone (needed for #6436).

Also, it's really confusing depending on the order of calls to have self.setup_env and self.build_env set. Wasn't able to remove completely self.setup_env, we still use it outside the function that creates the environment.

I've moved/modified the signals to be run just before the first command and send the current environment to execute over the ssh commands.

I still need to test this locally and see if it doesn't break anything else.

stsewd added 2 commits Dec 10, 2019
Currently when `sync_repository_task` gets triggered
we use a LocalEnvironment as environment.

We want to be explicit about respecting the docker setting
in all places where we do a clone.

Also, it's really confusing depending on the order of calls to have
self.setup_env set. Wasn't able to remove it completely, we still use it
outside the function that creates the environment.
self.version.slug,
# When called from ``SyncRepositoryTask.run`` we don't have
# a ``setup_env`` so we use just ``None`` and commands won't
# be recorded

This comment has been minimized.

Copy link
@stsewd

stsewd Dec 10, 2019

Author Member

Now we create the environment with record=False when using it from SyncRepositoryTask, we don't need this hack anymore.

@stsewd

This comment has been minimized.

Copy link
Member Author

stsewd commented Dec 10, 2019

Ok, triggering a build doesn't fail. All cool. I think this should be better if we use the class as it should be... Like passing all the necessary data to the constructor instead of the method and setting all the self attributes in there. But a larger refactor.

Opened #6452

@stsewd stsewd requested a review from readthedocs/core Dec 10, 2019
@@ -553,6 +569,10 @@ def run_build(self, docker, record):
)

try:
before_build.send(
sender=self.version,
environment=self.build_env,

This comment has been minimized.

Copy link
@humitos

humitos Dec 16, 2019

Member

Shouldn't we use the environment variable here to make consistency with the setup_env.

This comment has been minimized.

Copy link
@stsewd

stsewd Dec 16, 2019

Author Member

We don't have the environment var here, I didn't refactor build_env (there are a lot of places that depend on the order call), better to refactor it with #6452 instead of passing it around each method.

@stsewd stsewd requested a review from readthedocs/core Dec 16, 2019
Copy link
Member

ericholscher left a comment

This makes sense to me. It does feel half finished, but that's because we're working through a refactor, so I think it's fine for now 👍

@stsewd stsewd merged commit 8798752 into readthedocs:master Dec 17, 2019
2 checks passed
2 checks passed
continuous-documentation/read-the-docs Read the Docs build succeeded!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@stsewd stsewd deleted the stsewd:decouple-setup-env branch Dec 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.