Skip to content

Faster validation scripts#27964

Merged
LK4D4 merged 2 commits intomoby:masterfrom
dnephin:faster-validate
Nov 3, 2016
Merged

Faster validation scripts#27964
LK4D4 merged 2 commits intomoby:masterfrom
dnephin:faster-validate

Conversation

@dnephin
Copy link
Copy Markdown
Member

@dnephin dnephin commented Nov 1, 2016

Extracted from #27359

The hack/make framework of creating bundle directories and requiring execution of scripts to be run from a driver (hack/make.sh) is ok for build tasks that produce artifacts, but has little to no benefit for these validation/check scripts. This PR changes them to source all their dependencies directly, so they no longer require the hack/make.sh driver. It also moves them from hack/make to hack/validate so that it's clear they can be run directly without the driver.

They also run as a single process now, so instead of every one of them having to perform a git fetch, hack/validate/all does a single git fetch, reducing the overall runtime of validation.

Copy link
Copy Markdown
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

@dnephin some files changed permissions; can you check if that was intentional?

@thaJeztah
Copy link
Copy Markdown
Member

ping @tianon @vdemeester PTAL ❤️

Copy link
Copy Markdown
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

LGTM 🐸

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

❤️

Makefile Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Shouldn't all be rename default and having a all that runs default and `vendor ? 👼

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

hmm, that could work too

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I agree that all is a bit misleading if it doesn't include vendor 😅

Copy link
Copy Markdown
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

nevermind, answering my earlier question; see why they're changed :-) "approving" to neutralize my earlier "request changes"

Copy link
Copy Markdown
Member

@tianon tianon left a comment

Choose a reason for hiding this comment

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

This is excellent! ❤️ ❤️

Makefile Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I agree that all is a bit misleading if it doesn't include vendor 😅

@lowenna
Copy link
Copy Markdown
Member

lowenna commented Nov 2, 2016

LGTM. @dnephin Can you open an issue and assign to me once this is merged to cleanup the Windows CI scripts and remove the workarounds. Thanks.

@thaJeztah
Copy link
Copy Markdown
Member

@dnephin can you rename all, per #27964 (comment), then I think we're ready to go

Allow each script to run directly without the hack/make.sh wrapper. These
scripts do not produce artifacts and do not benefit from the "bundles"
framework.

Signed-off-by: Daniel Nephin <dnephin@docker.com>
Signed-off-by: Daniel Nephin <dnephin@docker.com>
@dnephin
Copy link
Copy Markdown
Member Author

dnephin commented Nov 3, 2016

Cool, I moved all -> default and created a new all that is really everything (for make validate).

Also rebased

Copy link
Copy Markdown
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

Still and even more LGTM 👼

@LK4D4 LK4D4 merged commit f54339d into moby:master Nov 3, 2016
@dnephin dnephin deleted the faster-validate branch November 3, 2016 22:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants