Skip to content

model: fast build fields to dockerInfo.buildDetails [ch1136]#888

Merged
maiamcc merged 5 commits into
masterfrom
maiamcc/fast-build
Dec 21, 2018
Merged

model: fast build fields to dockerInfo.buildDetails [ch1136]#888
maiamcc merged 5 commits into
masterfrom
maiamcc/fast-build

Conversation

@maiamcc

@maiamcc maiamcc commented Dec 21, 2018

Copy link
Copy Markdown
Contributor

Hello @jazzdan, @nicks,

Please review the following commits I made in branch maiamcc/fast-build:

8d94a3f (2018-12-21 16:10:23 -0500)
model: fast build fields to dockerInfo.buildDetails

Code review reminders, by giving a LGTM you attest that:

  • Commits are adequately tested
  • Code is easy to understand and conforms to style guides
  • Incomplete code is marked with TODOs
  • Code is suitably instrumented with logging and metrics

@maiamcc
maiamcc requested review from jazzdan and nicks December 21, 2018 21:10
Comment thread internal/model/docker_info.go Outdated
func (FastBuild) buildDetails() {}
func (fb FastBuild) Empty() bool { return reflect.DeepEqual(fb, FastBuild{}) }

// func (m1 Manifest) stepsEqual(s2 []Step) bool {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👀

@jazzdan jazzdan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!

@@ -104,24 +97,33 @@ func (m Manifest) Dockerignores() []Dockerignore {
func (m Manifest) LocalPaths() []string {
if sbInfo := m.StaticBuildInfo(); !sbInfo.Empty() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we transform this in to a case statement?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

not really, cuz we're looking at the type of two different fields :-/

Comment thread internal/model/manifest.go Outdated
return fmt.Errorf(
"[validate] mount.LocalPath must be an absolute path (got: %s)", m.LocalPath)

if fbInfo := m.FastBuildInfo(); !fbInfo.Empty() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is a lot of indentation, could we move the check about?

if fbInfo := m.FastBuildInfo(); fbInfo.Empty() {
    return nil
}
// ... 

@maiamcc
maiamcc merged commit 222dfc5 into master Dec 21, 2018
@maiamcc
maiamcc deleted the maiamcc/fast-build branch December 21, 2018 23:16
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