Skip to content

Carry #27834 — Do not require .git in the build context#28473

Closed
vdemeester wants to merge 2 commits intomoby:masterfrom
vdemeester:carry-pr-27834
Closed

Carry #27834 — Do not require .git in the build context#28473
vdemeester wants to merge 2 commits intomoby:masterfrom
vdemeester:carry-pr-27834

Conversation

@vdemeester
Copy link
Copy Markdown
Member

@vdemeester vdemeester commented Nov 16, 2016 •
edited
Loading

Carry #27834 and hopefully fixes builds 👼.

  • Add .git to .dockerignore
  • export DOCKER_GITCOMMIT in the Makefile
  • prioritize DOCKER_GITCOMMIT against the git command in
    ./hack/make.sh
  • Also add integration-cli/bundles to gitignore (it's generated when
    using integration-cli shell)

Note that, on my laptop for example, I'm sending 30M of context with this, and 150M without 🙀, so I really think this can be a huge win 👼

/cc @tianon @jhowardmsft @icecrime @thaJeztah @cpuguy83

🐸

@vdemeester vdemeester mentioned this pull request Nov 16, 2016
Closed
@vdemeester vdemeester added status/2-code-review status/failing-ci Indicates that the PR in its current state fails the test suite and removed status/0-triage labels Nov 16, 2016
@vdemeester
Copy link
Copy Markdown
Member Author

vdemeester commented Nov 16, 2016

Hum, I really don't know why we're not using make … on the CI and instead manual docker run …, really 😓

AkihiroSuda
AkihiroSuda reviewed Nov 16, 2016
View reviewed changes
.dockerignore Outdated
Copy link
Copy Markdown
Member

@AkihiroSuda AkihiroSuda Nov 16, 2016

Choose a reason for hiding this comment

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

Just a thought: I think this directory should be removed and unified to the top-level bundle directory, but probably it should be another PR

@tianon
Copy link
Copy Markdown
Member

tianon commented Nov 16, 2016

Hum, I really don't know why we're not using make … on the CI and instead manual docker run …, really 😓

IIRC, it was a security concern -- if the CI were using the Makefile directly, I could send a PR which simply updates the Makefile with some malicious actions and they'll be run directly on the host. By forcing the CI to use docker build ... && docker run ... directly, we insulate the CI from that vector.

@vdemeester
Copy link
Copy Markdown
Member Author

vdemeester commented Nov 16, 2016

@tianon hum I see 😓

@vdemeester
Copy link
Copy Markdown
Member Author

vdemeester commented Nov 22, 2016

Waiting for the 1.13 release to update CI… I wouldn't want to break anything right now 👼

@vdemeester vdemeester added this to the 1.14.0 milestone Nov 22, 2016
John Howard and others added 2 commits December 25, 2016 22:33
@vdemeester
Add .git to .dockerignore
c245e7a 
Signed-off-by: John Howard <jhoward@microsoft.com>
@vdemeester
Do not required .git in the build context
3e9f248 
- export DOCKER_GITCOMMIT in the Makefile
- prioritize DOCKER_GITCOMMIT against the `git` command in
  `./hack/make.sh`
- Also add `integration-cli/bundles` to gitignore (it's generated when
  using integration-cli shell)

Signed-off-by: Vincent Demeester <vincent@sbr.pm>
@icecrime
Copy link
Copy Markdown
Contributor

icecrime commented Dec 28, 2016

@vdemeester Is it ok to close this one until 1.13.0 is out of the door?

@vdemeester
Copy link
Copy Markdown
Member Author

vdemeester commented Jan 6, 2017

Right, I'll reopen a little bit later 👼

@vdemeester vdemeester closed this Jan 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AkihiroSuda AkihiroSuda AkihiroSuda left review comments

Assignees

No one assigned

Labels

status/failing-ci Indicates that the PR in its current state fails the test suite status/2-code-review

Projects

None yet

Milestone

17.04.0

Development

Successfully merging this pull request may close these issues.

5 participants

@vdemeester @tianon @icecrime @AkihiroSuda @GordonTheTurtle