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

feat: parse healthcheck in dockerfile #971

Merged
merged 6 commits into from Jun 3, 2020
Merged

feat: parse healthcheck in dockerfile #971

merged 6 commits into from Jun 3, 2020

Conversation

seongm-1
Copy link
Contributor

@seongm-1 seongm-1 commented May 27, 2020

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

internal/pkg/docker/dockerfile/dockerfile.go Outdated Show resolved Hide resolved
internal/pkg/docker/dockerfile/dockerfile.go Outdated Show resolved Hide resolved
internal/pkg/docker/dockerfile/dockerfile.go Outdated Show resolved Hide resolved
internal/pkg/docker/dockerfile/dockerfile.go Outdated Show resolved Hide resolved
internal/pkg/docker/dockerfile/dockerfile.go Outdated Show resolved Hide resolved
internal/pkg/docker/dockerfile/dockerfile.go Outdated Show resolved Hide resolved
internal/pkg/docker/dockerfile/dockerfile.go Outdated Show resolved Hide resolved
internal/pkg/docker/dockerfile/dockerfile.go Outdated Show resolved Hide resolved
internal/pkg/docker/dockerfile/dockerfile.go Outdated Show resolved Hide resolved
internal/pkg/docker/dockerfile/dockerfile.go Outdated Show resolved Hide resolved
Copy link
Contributor

@efekarakus efekarakus left a comment

Choose a reason for hiding this comment

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

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.

internal/pkg/docker/dockerfile/dockerfile.go Outdated Show resolved Hide resolved
internal/pkg/docker/dockerfile/dockerfile.go Outdated Show resolved Hide resolved
@@ -80,7 +105,7 @@ func (df *Dockerfile) GetExposedPorts() ([]uint16, error) {
return ports, err
}

// 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
Copy link
Contributor

Choose a reason for hiding this comment

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

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 :)

internal/pkg/docker/dockerfile/dockerfile.go Outdated Show resolved Hide resolved
internal/pkg/docker/dockerfile/dockerfile.go Outdated Show resolved Hide resolved
internal/pkg/docker/dockerfile/dockerfile.go Outdated Show resolved Hide resolved
internal/pkg/docker/dockerfile/dockerfile.go Outdated Show resolved Hide resolved
internal/pkg/docker/dockerfile/dockerfile.go Show resolved Hide resolved
internal/pkg/docker/dockerfile/dockerfile.go Outdated Show resolved Hide resolved
internal/pkg/docker/dockerfile/dockerfile_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@efekarakus efekarakus left a comment

Choose a reason for hiding this comment

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

Looks great 🚀 tiny nits! once addressed feel free to remove the "do-not-merge" label and mergify can auto-merge after 2 approvals 😁.

internal/pkg/docker/dockerfile/dockerfile.go Outdated Show resolved Hide resolved
internal/pkg/docker/dockerfile/dockerfile.go Outdated Show resolved Hide resolved
internal/pkg/docker/dockerfile/dockerfile_test.go Outdated Show resolved Hide resolved
@efekarakus efekarakus added the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Jun 3, 2020
inst := fmt.Sprint(instruction)

// Getting the value at a children will return the Dockerfile directive
switch d := child.Value; d {
Copy link
Contributor

Choose a reason for hiding this comment

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

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
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Talked about it offline and it provided a compiler error. Might have to tackle this later on.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@iamhopaul123 iamhopaul123 left a comment

Choose a reason for hiding this comment

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

🚢

@seongm-1 seongm-1 removed the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Jun 3, 2020
@mergify mergify bot merged commit 5b87893 into aws:master Jun 3, 2020
Sprint 🏃‍♀️ automation moved this from In review to Pending release Jun 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Sprint 🏃‍♀️
  
Pending release
Development

Successfully merging this pull request may close these issues.

None yet

3 participants