feat: parse healthcheck in dockerfile#971
feat: parse healthcheck in dockerfile#971mergify[bot] merged 6 commits intoaws:masterfrom seongm-1:healthcheck-parser
Conversation
efekarakus
left a comment
There was a problem hiding this comment.
Great stuff! mostly some Go best-practices.
Do we need a GetHealthcheck() (*HealthCheck, error) method? I think this PR parses the content but doesn't provide a way for clients to access the HealthCheck.
| } | ||
|
|
||
| // parse takes a Dockerfile and fills in struct members based on methods like parseExpose and (TODO) parseHealthcheck | ||
| // parse takes a Dockerfile and fills in struct members based on methods like parseExpose and parseHealthcheck |
There was a problem hiding this comment.
https://github.com/golang/go/wiki/CodeReviewComments#comment-sentences
Comments should begin with the name of the thing being described and end in a period
This wiki page is pretty neat and contains all sorts of best-practices, if you'd like to take a look when you've time :)
efekarakus
left a comment
There was a problem hiding this comment.
Looks great 🚀 tiny nits! once addressed feel free to remove the "do-not-merge" label and mergify can auto-merge after 2 approvals 😁.
| inst := fmt.Sprint(instruction) | ||
|
|
||
| // Getting the value at a children will return the Dockerfile directive | ||
| switch d := child.Value; d { |
There was a problem hiding this comment.
I really like what you did with ParseInstruction! I just realized that the instruction itself is already parsed into a moby type and we don't need to parse it again :D
Can try something like this:
var hc *HealthCheck
switch t := instruction.(type); t {
case *instructions.ExposeCommand:
// Read fields from t.
case *instructions.HealthCheckCommand:
hc = &HealthCheck{
interval: uint16(t.Health.Interval.Seconds()),
// ... same for other fields
}
}There was a problem hiding this comment.
Talked about it offline and it provided a compiler error. Might have to tackle this later on.
There was a problem hiding this comment.
Sounds good but I think it's worthwhile addressing it because it will significantly remove code from us and will keep it in sync with how Docker parses the file.
The ability to parse Healthcheck options from a user's dockerfile.
Also added, some of Moby buildkit packages for ease of parsing dockerfile.
https://godoc.org/github.com/moby/buildkit/frontend/dockerfile/parser
https://godoc.org/github.com/moby/buildkit/frontend/dockerfile/instructions
Addresses #938
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Binary Size (added Mobykit parsers)
Before: 44.2 MB
After: 44.4 MB