Build: pass api_client down to environment/builders/etc #10390
Conversation
For #10289 we won't have the credentials hardcoded in our settings anymore, but instead we will be provided with a dynamic token for the build to use, for this reason we now have to pass the api client object around.
| self.api_client = api_client | ||
|
|
||
| if self.record and not self.api_client: | ||
| raise ValueError("api_client is required when record=True") |
There was a problem hiding this comment.
Solid defensive programming 👍
| @@ -167,33 +166,23 @@ def get_config_params(self): | |||
| versions = [] | |||
| downloads = [] | |||
| subproject_urls = [] | |||
| # Avoid hitting database and API if using Docker build environment | |||
| if settings.DONT_HIT_API: | |||
There was a problem hiding this comment.
We have DONT_HIT_API and DONT_HIT_DB settings, which seem like inverses of the same value 🙃
Does it make sense to try to remove the DONT_HIT_API & DONT_HIT_DB settings as a later refactor here, or are they still needed in other places in the codebase?
There was a problem hiding this comment.
Yeah, I think we should just always assume that builders hit our API, and the rest of our code the DB.
Looks like we can just remove DONT_HIT_API, I can try to remove DONT_HIT_DB in another PR.
There was a problem hiding this comment.
Yes. I think these variables where only useful when we didn't have Docker development setup.
| @@ -227,7 +226,7 @@ def get_command(self): | |||
| return ' '.join(self.command) | |||
| return self.command | |||
|
|
|||
| def save(self): | |||
| def save(self, api_client): | |||
There was a problem hiding this comment.
I assume it's easier to pass it in here, than to put the client on the BuildCommand when it's created?
There was a problem hiding this comment.
it touches less code, and the API client is scoped to where is needed (just this method), since commands can be optionally saved.
For #10289 we won't have the credentials hardcoded in our settings anymore, but instead we will be provided with a dynamic token for the build to use, for this reason we now have to pass the api client object around.
We were using the
DONT_HIT_APIjust for testing, instead of doing that, I'm just mocking the api client.