fix(cloudbuilder): add error handling to fallback to cli install of buildx builder #6258
Conversation
| await this.remove_builder() | ||
| await this.remove_tmpdir() |
There was a problem hiding this comment.
Does the order matter? If so, can we add a comment to that effect since this could easily break.
(Ideally we have a test for this but I don't know how feasible that is here.)
There was a problem hiding this comment.
Should we always use camelCase for method names? :)
There was a problem hiding this comment.
You are right, we should add a comment since it matters because if we fallback to removing using docker cli in remove_builder we assume the certDir still exists and we use it as cwd. Though i don't see why we would need to change to the certDir to run that command, will just remove this dependency.
| // An error is thrown e.g. if the path does not exist. | ||
| // We don't need to handle this error, as we will fall back to the CLI installation. | ||
| if (isErrnoException(e) && e.code === "ENOENT") { | ||
| return false |
There was a problem hiding this comment.
Should we still log the error though? In case there are other failure modes we're not thinking of which we won't see if the error is swallowed.
What this PR does / why we need it:
We try to install a buildx builder directly by writing to the local .docker directory and if that fails we use docker cli to install the builder. In some environments e.g. CI the local .docker directory may not be available or not configured with the correct subpath of
~/.docker/buildx/instances. We were missing some error handling and await when checking for these paths, which led garden to fail in case the path does not exist.Also adds an error message for a common error which is missing or wrong credentials for the registry.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer: