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

Proposal: reorganize hack/ build scripts #35937

Closed
dnephin opened this issue Jan 4, 2018 · 3 comments
Closed

Proposal: reorganize hack/ build scripts #35937

dnephin opened this issue Jan 4, 2018 · 3 comments

Comments

@dnephin
Copy link
Member

dnephin commented Jan 4, 2018

Problem Statement

The organization of build scripts in hack/ has remained largely unchanged for a long time. This repo has undergone significant changes and the build scripts could use a refresh.

Goals

  • remove hack/make.sh so that scripts can be executed directly without a driver
  • keep a clear distinction between libraries (currently files prefixed with .) and "task scripts"
  • group the scripts by high level operation (build, test, release, etc)
  • remove anything that is no longer used
  • support upcoming changes to the repo, (Rename binaries to moby-* #34226 and Completing the Moby transition #35115)

Proposal

  • remove hack/make.sh, and move everything out of hack/make to a new location under hack
  • any setup logic in hack/make.sh will move to a library under hack/lib or hack/<category>/lib

Structure:

hack
|--- ci        # CI container entrypoints, one script per CI build (already exists)
|--- run       # scripts for running the daemon (ex dind, hack/make/run)
|--- release   # scripts for releasing new versions (ex: generate-authors.sh)
|--- build     # scripts for building the binaries (hack/make/binary*, hack/make/cross)
     `--- generate # generate code (generate-swagger-api.sh, hack/make/.go-autogen)
|--- test     # scripts for running test suites (hack/test already exists)
|--- validate # scripts for validating code, linting, etc (already exists)
|--- deps     # installing dependencies, (vendor.sh, and hack/dockerfile/)
|--- libs 

Anything in libs should only define functions, nothing should run when sourced. Each directory can also have a libs directory for anything that is not general enough to be at the top level

(hack/integration-cli-on-swarm/ could be moved back to contrib/ since it's mostly go code, or would be under hack/test/integration-cli-on-swarm/)

@tianon
Copy link
Member

tianon commented Jan 4, 2018

This proposed structure sounds really sane to me! Thanks for thinking all this through and writing up this proposal. ❤️

Would these scripts all assume they're being launched from the root of the repo like hack/make.sh does now? (seems like a sane/safe assumption to me -- simplifies writing the scripts and actually using those libs files in sane ways from the other scripts without doing hacky things with $BASH_SOURCE and/or $0 -- just want to make sure we actually talk about it explicitly and perhaps even document it in this new hack structure)

I have some concerns about hack/ci -- I know at least a while back, we used to only run CI scripts that come from this repo from within the context of a built image (ala docker run image-built-from-pr-dockerfile hack/make.sh ...) to help insulate the CI machines from malicious PRs (things like starting a bitcoin miner in a script in hack/ci, making a PR, then reaping the benefits, or perhaps even more malicious things). Would these be similar, or are these intended to be run in the CI as-is? Do we have other mitigation strategies for malicious PRs?

@dnephin
Copy link
Member Author

dnephin commented Jan 4, 2018

Would these scripts all assume they're being launched from the root of the repo like hack/make.sh does now?

Yes, I think this is a fair assumption. Good to call it out, thanks!

I have some concerns about hack/ci

We're already using these today, I forgot to document that they already exist. They are indeed just the container entrypoint, as it was in the past with hack/make.sh. I've updated the comment to reflect this.

@cpuguy83
Copy link
Member

cpuguy83 commented Jan 5, 2018

to help insulate the CI machines from malicious PRs

I know kube (or at least some kube projects) require a reviewer to comment something like /run-all-thet-tests (or whatever) before CI kicks off. Could be worth adopting it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants