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

added healthcheck to config struct and healthcheck struct #138

Merged
merged 3 commits into from
May 14, 2018
Merged

added healthcheck to config struct and healthcheck struct #138

merged 3 commits into from
May 14, 2018

Conversation

chrisz100
Copy link
Contributor

Needed to implement HEALTHCHECK commands to kaniko - see GoogleContainerTools/kaniko#17 (comment)

@chrisz100
Copy link
Contributor Author

@dlorenc cc

// {"NONE"} : disable healthcheck
// {"CMD", args...} : exec arguments directly
// {"CMD-SHELL", command} : run command with system's default shell
Test []string `json:",omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: should these be lowercased like json:"test,omitempty"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the original implementation from docker - moby/moby
What would adding the test to the json tag change here specifically?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, basically just to make it lowercase when marshalled, to match the way we marshal all the other values in this file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

like that?

@dlorenc
Copy link
Contributor

dlorenc commented May 14, 2018

Ah sorry for the back and forth, my coworkers point out that we actually should leave it the way you had it before to match exactly how Docker serializes. After that, this LGTM.

@dlorenc dlorenc merged commit ee5a6c2 into google:master May 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants