Add Buildah as an alternative container image build tool #559
Conversation
|
|
|
cc @gorkem |
There was a problem hiding this comment.
This is a really nice feature and I like the way you've done it. I'm afraid I do have a few nitpicks - we definitely need to address the config design, and it would be great if we could have a think about how we detect whether to fall back to Docker, but that's not so crucial. Thanks for the PR!
package.json
Outdated
| @@ -231,6 +231,10 @@ | |||
| "resource-commands-on-files": { | |||
| "type": "boolean", | |||
| "description": "If true, show Kubernetes resource commands on file context menu for all YAML files" | |||
| }, | |||
| "preferBuildah": { | |||
There was a problem hiding this comment.
Minor: suggest adding a default for this in the defaults section
src/imageBuild/buildToolsRegistry.ts
Outdated
| } | ||
|
|
||
| /** Currently used image build tool. */ | ||
| export let currentBuildTool: string; |
There was a problem hiding this comment.
Prefer not to export a mutable variable. If other modules need it then maybe export a function that returns it?
src/imageBuild/buildToolsRegistry.ts
Outdated
| export let currentBuildTool: string; | ||
|
|
||
| /** Starts listening changing the preferred build tool. */ | ||
| export function start(): vscode.Disposable { |
There was a problem hiding this comment.
What would be the cost of just looking up the config entry each time, instead of having a mutable variable and an event listener? We do this throughout the extension and it feels simpler and doesn't seem to cause performance problems (he said cautiously, acutely aware that we do have performance problems).
There was a problem hiding this comment.
Oh, it's because it's not just looking in config, it's also checking if buildah is present. And we definitely don't want to do that every time!
There was a problem hiding this comment.
For most tools we defer detection until the user tries to use the tool. I think we check for kubectl on startup but that is just to warn - we don't rely on it for a setting. We also have existing mechanisms for checking if a binary is present (see e.g. binutil.ts) which might be faster than calling the executable.
src/imageBuild/buildToolsRegistry.ts
Outdated
|
|
||
| /** Starts listening changing the preferred build tool. */ | ||
| export function start(): vscode.Disposable { | ||
| detectCurrentBuildTool(); |
There was a problem hiding this comment.
This makes me anxious... in practice it should never be an issue, but in theory detectCurrentBuildTool could take a long time to run on extension startup, and so currentBuildTool could be accessed before it had been set.
package.json
Outdated
| @@ -231,6 +231,10 @@ | |||
| "resource-commands-on-files": { | |||
| "type": "boolean", | |||
| "description": "If true, show Kubernetes resource commands on file context menu for all YAML files" | |||
| }, | |||
| "preferBuildah": { | |||
There was a problem hiding this comment.
I am not sure this is the right design here - what if someone comes along with a PR to support another build tool, say img? How would we manage the interaction between preferBuildah and a hypothetical preferImg?
Suggest instead a preferredImageBuildTool with an enum that restricts it to be Docker or Buildah (cf. the kubectlVersioning config setting).
src/imageBuild/buildToolsRegistry.ts
Outdated
| import { getPreferBuildah } from '../components/config/config'; | ||
| import { shell } from '../shell'; | ||
|
|
||
| interface ContainerImageBuildTool { |
There was a problem hiding this comment.
I like the way you've done this - should make it easy if people want to add other build tools in future.
|
Hello @itowlson, thank you for your review!
|
|
Could you check the status of your CLA and sign if you haven't already done so please? Unfortunately we can't take this PR without a CLA. |
|
Thank you @azatsarynnyy for the CLA sign-off - we have lift-off! |
In this PR I propose to add Buildah support as an alternative container image build tool that is used for basic image operations by
Kubernetes: RunandKubernetes: Debugcommands.By default, Docker is used. And if the user prefers using Buildah it can be enabled with
preferBuildahconfiguration.