Skip to content
This repository was archived by the owner on Feb 5, 2020. It is now read-only.

images: Pin bazel to specific version in docker image #3239

Merged
trawler merged 4 commits into from
May 22, 2018
Merged

images: Pin bazel to specific version in docker image #3239

trawler merged 4 commits into from
May 22, 2018

Conversation

@trawler
Copy link
Contributor

@trawler trawler commented May 17, 2018

fixes #INST-1058

@coreosbot
Copy link

coreosbot commented May 17, 2018

Can one of the admins verify this patch?

@trawler trawler requested review from alexsomesan, mxinden and squat May 17, 2018 13:44
squat
squat suggested changes May 17, 2018
View reviewed changes
Copy link
Contributor

@squat squat left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@trawler this looks good so far. We also need to build the image, push it to docker, and update the tag in the Jenkinsfile. Once that is done, we need to trigger smoke tests to make sure everything still builds correctly 👍

alexsomesan
alexsomesan previously approved these changes May 17, 2018
View reviewed changes
Copy link
Contributor

@alexsomesan alexsomesan left a comment

Choose a reason for hiding this comment

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

Looks good, but also what @squat said.

@trawler trawler force-pushed the pin_bazel_version_to_docker_image branch from e6ebf99 to 2639ebd Compare May 18, 2018 10:54
@trawler trawler force-pushed the pin_bazel_version_to_docker_image branch from 2639ebd to 5d1960e Compare May 18, 2018 11:01
@trawler
Copy link
Contributor Author

trawler commented May 18, 2018

ok to test

@trawler
Copy link
Contributor Author

trawler commented May 22, 2018

retest this please

enxebre
enxebre reviewed May 22, 2018
View reviewed changes
images/tectonic-builder/Dockerfile Outdated
RUN go get github.com/jstemmer/go-junit-report

### Tools used by 'make structure-check'
RUN go get github.com/segmentio/terraform-docs
Copy link
Contributor

@enxebre enxebre May 22, 2018

Choose a reason for hiding this comment

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

this should probably go away as well

enxebre
enxebre reviewed May 22, 2018
View reviewed changes
images/tectonic-builder/Dockerfile.bazel
&& curl -LO "https://github.com/bazelbuild/bazel/releases/download/${BAZEL_VERSION}/bazel_${BAZEL_VERSION}-linux-x86_64.deb" \
&& dpkg -i bazel_*.deb

RUN apt --fix-broken install -y
Copy link
Contributor

@enxebre enxebre May 22, 2018

Choose a reason for hiding this comment

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

can you clarify why do we need the additional libraries above and this last RUN?

Copy link
Contributor Author

@trawler trawler May 22, 2018

Choose a reason for hiding this comment

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

Google removes all older bazel versions from their apt repo. So in order to fetch older versions we need to install it from github. The last RUN is supposed to install all the declared dependencies in the deb file.

Copy link
Contributor Author

@trawler trawler May 22, 2018

Choose a reason for hiding this comment

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

the additional libraries are required per the documentation: https://docs.bazel.build/versions/master/install-ubuntu.html

enxebre
enxebre previously approved these changes May 22, 2018
View reviewed changes
spangenberg
spangenberg previously approved these changes May 22, 2018
View reviewed changes
@trawler trawler dismissed stale reviews from spangenberg and enxebre via b37e350 May 22, 2018 11:56
@trawler trawler merged commit 71ac693 into coreos:master May 22, 2018
@wking wking mentioned this pull request Jul 25, 2018
Merged
wking added a commit to wking/openshift-installer that referenced this pull request Jul 25, 2018
@wking
images/tectonic-builder/Dockerfile: Drop stale installer 'rm -rf'
315bd67 
1a49fbb (Move structure-check tools from Jenkinsfile to builder
image, 2017-09-12, coreos/tectonic-installer#1889), added a:

  RUN go get github.com/s-urbaniak/terraform-examples

to this Dockerfile.  That package moved into the installer repository,
but all we need for generation/testing is the terraform-examples
binary, so a68555d (tests/ci: fix dockerfile and remove some layers,
2017-12-19, coreos/tectonic-installer#2600) removed the associated
source with:

  RUN go get github.com/coreos/tectonic-installer/contrib/terraform-examples && \
    rm -rf /go/src/github.com/coreos/tectonic-installer/

(i.e. "download
github.com/coreos/tectonic-installer/contrib/terraform-examples, build
terraform-examples, install it into $GOBIN, and remove the source").

d61abd4 (*: cleanup bazel rules, 2018-03-26,
coreos/tectonic-installer#3137) removed the Makefile which had been
calling terraform-examples, and 7c73c34 (remove terraform-examples
from the tectonic-builder image, 2018-05-18,
coreos/tectonic-installer#3239) removed the terraform-examples install
from this Dockerfile.  With the terraform-examples install removed, Go
will no longer be downloading that source, so there's no longer any
reason to remove it.  This commit removes the unnecessary removal,
which should make Dockerfile builds a bit faster and save an empty
layer in the resulting images.  More importantly, it avoids
distracting future devs reading the Dockerfile source ;).
frobware pushed a commit to frobware/installer that referenced this pull request Sep 17, 2018
@wking @frobware
images/tectonic-builder/Dockerfile: Drop stale installer 'rm -rf'
9e701ca 
1a49fbb (Move structure-check tools from Jenkinsfile to builder
image, 2017-09-12, coreos/tectonic-installer#1889), added a:

  RUN go get github.com/s-urbaniak/terraform-examples

to this Dockerfile.  That package moved into the installer repository,
but all we need for generation/testing is the terraform-examples
binary, so a68555d (tests/ci: fix dockerfile and remove some layers,
2017-12-19, coreos/tectonic-installer#2600) removed the associated
source with:

  RUN go get github.com/coreos/tectonic-installer/contrib/terraform-examples && \
    rm -rf /go/src/github.com/coreos/tectonic-installer/

(i.e. "download
github.com/coreos/tectonic-installer/contrib/terraform-examples, build
terraform-examples, install it into $GOBIN, and remove the source").

d61abd4 (*: cleanup bazel rules, 2018-03-26,
coreos/tectonic-installer#3137) removed the Makefile which had been
calling terraform-examples, and 7c73c34 (remove terraform-examples
from the tectonic-builder image, 2018-05-18,
coreos/tectonic-installer#3239) removed the terraform-examples install
from this Dockerfile.  With the terraform-examples install removed, Go
will no longer be downloading that source, so there's no longer any
reason to remove it.  This commit removes the unnecessary removal,
which should make Dockerfile builds a bit faster and save an empty
layer in the resulting images.  More importantly, it avoids
distracting future devs reading the Dockerfile source ;).
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@mxinden mxinden Awaiting requested review from mxinden

4 more reviewers

@enxebre enxebre enxebre approved these changes

@squat squat squat approved these changes

@spangenberg spangenberg spangenberg left review comments

@alexsomesan alexsomesan alexsomesan left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

platform/aws run-smoke-tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@trawler @coreosbot @spangenberg @alexsomesan @enxebre @squat