Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Feature/healthcheck signal #177

Merged

Conversation

chrisz100
Copy link
Contributor

Solves #17

@@ -68,6 +68,8 @@ func GetCommand(cmd instructions.Command, buildcontext string) (DockerCommand, e
return &ArgCommand{cmd: c}, nil
case *instructions.ShellCommand:
return &ShellCommand{cmd: c}, nil
case *instructions.HealthCheckCommand:
return &HealthCheckCommand{cmd: c}, nil
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the build error, it looks like you'll have to translate from the dockerfile HealthCheck type to the go-containerregistry one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exactly. Just looking into how to do this the most efficiently.

Just creating a v1.HealthConfig and passing all values would work, but I wouldn't really regard this as future proof or clean tbh. Any alternatives you could think of?

@chrisz100
Copy link
Contributor Author

So... I don't really like this way of doing it but it works and there is no way to convert struct to struct more fail-safe/ autonomous that I am aware of.

@dlorenc
Copy link
Collaborator

dlorenc commented May 14, 2018

So... I don't really like this way of doing it but it works and there is no way to convert struct to struct more fail-safe/ autonomous that I am aware of.

Hmm, if the structs are actually equivalent you might be able to cast them: https://beta.golang.org/doc/go1.8#language

That would then error if the types don't match which would probably be a good thing.

@chrisz100
Copy link
Contributor Author

This is the case as of now as well sadly:
config.Healthcheck = v1.HealthConfig(h.cmd.Health)

pkg/commands/healthcheck.go:44:38: cannot convert h.cmd.Health (type *container.HealthConfig) to type "github.com/GoogleContainerTools/kaniko/vendor/github.com/google/go-containerregistry/v1".HealthConfig
make: *** [out/executor] Error 2

On go 1.10

@dlorenc
Copy link
Collaborator

dlorenc commented May 14, 2018

Hmm, that might be because one is a pointer and the other isn't. Could you give it one more try by dereferencing the h.cmd.Health before converting?

@chrisz100
Copy link
Contributor Author

chrisz100 commented May 14, 2018

Same thing:

test := &h.cmd.Health
converted := v1.HealthConfig(test)

--> 
pkg/commands/healthcheck.go:45:30: cannot convert test (type **container.HealthConfig) to type "github.com/GoogleContainerTools/kaniko/vendor/github.com/google/go-containerregistry/v1".HealthConfig
make: *** [out/executor] Error 2

EDIT:
My bad. That looks better!

@dlorenc
Copy link
Collaborator

dlorenc commented May 14, 2018

Nice!

@dlorenc dlorenc merged commit fbe3e05 into GoogleContainerTools:master May 14, 2018
@chrisz100 chrisz100 deleted the feature/healthcheck_signal branch May 15, 2018 05:52
@priyawadhwa priyawadhwa added the area/dockerfile-command For all bugs related to dockerfile file commands label Feb 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/dockerfile-command For all bugs related to dockerfile file commands
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants