Be explicit when using setup_env #6451
Conversation
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 |
There was a problem hiding this comment.
Now we create the environment with record=False when using it from SyncRepositoryTask, we don't need this hack anymore.
|
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 |
| @@ -553,6 +569,10 @@ def run_build(self, docker, record): | |||
| ) | |||
|
|
|||
| try: | |||
| before_build.send( | |||
| sender=self.version, | |||
| environment=self.build_env, | |||
There was a problem hiding this comment.
Shouldn't we use the environment variable here to make consistency with the setup_env.
There was a problem hiding this comment.
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.
Currently, when
sync_repository_taskgets triggered we use aLocalEnvironmentas 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_envandself.build_envset. Wasn't able to remove completelyself.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.