image build support #122
image build support #122
Conversation
| }, | ||
| &DefParameter{ | ||
| Name: containerRegistryURL, | ||
| Title: "Kubernetes Images registry URL", |
There was a problem hiding this comment.
It's not exactly Kubernetes. Normally it must be a registry for any container runtime.
So it's more like Container Registry URL like a var name.
If it's not supported by Docker now (we just don't push image), leave a comment in description.
| VolumeFlags string | ||
| RegistryURL string | ||
| RegistryInsecure bool | ||
| RegistryType driver.KubernetesRegistry |
There was a problem hiding this comment.
It's a private structure which won't be used outside of the package. Let's make the fields private.
| case driver.Docker: | ||
| fallthrough | ||
| default: | ||
| //panic(fmt.Errorf("unsupported runtime type for cleanup: %s", c.rtype)) |
There was a problem hiding this comment.
Don't forget comments.
|
|
||
| // cleanupRuntimeResources handles cleanup in the correct order for each runtime type | ||
| func (c *runtimeContainer) cleanupRuntimeResources(ctx context.Context, cid string, a *Action, createOpts *driver.ContainerDefinition) { | ||
| switch c.rtype { |
There was a problem hiding this comment.
It doesn't seem ok that we distinguish container runtime types.
We may want to add new container runtimes and it's better that we have the implementation per type, not in switch.
Try to rework the interface and hide the implementation there.
It's ok if some method is empty.
| // cleanupKubernetesResources handles Kubernetes-specific cleanup order | ||
| func (c *runtimeContainer) cleanupKubernetesResources(ctx context.Context, cid string, a *Action, options driver.ImageOptions) { | ||
| // Remove image first if requested | ||
| if c.rcf.RemoveImg { |
There was a problem hiding this comment.
I understand there is a reason to remove image first.
But it seems completely off.
I suppose for docker case, maybe we need to follow kubernetes way:
- Delete image first. For docker we may want to specify
force. - If container was already removed, just ignore the error.
| }, | ||
| }) | ||
|
|
||
| launchr.Log().Debug("build output: ", "output", stdout.String()) |
There was a problem hiding this comment.
Let's be a bit more specific to the source of the log. Add some runtime information.
|
|
||
| launchr.Log().Debug("build output: ", "output", stdout.String()) | ||
| if err != nil { | ||
| return fmt.Errorf("error container exec: %w", err) |
There was a problem hiding this comment.
Let's be more specific in the error. Like failed to build image.
| return false, nil | ||
| } | ||
|
|
||
| return false, fmt.Errorf("error container exec: %w, message: %s", err, stdout.String()) |
There was a problem hiding this comment.
Let's have a more specific error, not just container exec.
| Force bool | ||
| RegistryType KubernetesRegistry | ||
| RegistryURL string | ||
| BuildContainerID string |
There was a problem hiding this comment.
This is very strange to have BuildContainerID for image remove options. They must be unrelated.
Build container id is a hardcoded value in k8s - meaning specific to implementation, not used in Docker or anywhere. Please, review.
Plus, you have the same struct fields in 2 structs.
Consider moving it to a separate stucture.
It's strange you have RegistryType in RemoveOptions.
For me, Options should contain how to remove, but not where.
Maybe, you should create a separate struct for Image related information and pass not Image string, but Image YourType
| return cid, nil | ||
| } | ||
|
|
||
| func (c *runtimeContainer) containerCreateKubernetes(ctx context.Context, a *Action, createOpts *driver.ContainerDefinition) (string, error) { |
There was a problem hiding this comment.
No, it's not ok to have a separate method per type.
All implementation must be done in the container related functionality.
Consider refactoring the interface to hide implementation inside.
A method can be empty if it is not relevant for example in Docker.
2 participants
No description provided.