tests: Add upstream terraform trigger by mxinden · Pull Request #1444 · coreos/tectonic-installer

mxinden · 2017-07-20T10:01:54Z

We are currently using a custom terraform version for our default CI
pipeline (See #1247).
As end users of Tectonic installer use upstream terraform we need to
test with upstream terraform as well. We are building and publishing the
Tectonic builder image both with CoreOS Terraform as well as upstream
Terraform. This is done via docker --build-arg TERRAFORM_URL. CoreOS
Terraform is used for normal PR and branch tests, upstream Terraform is
used once per day on master.

Add --build-arg TERRAFORM_URL to Tectonic builder Docker file
Update documentation in tectonic-builder/README.md
Add upstream-terraform-trigger.groovy pipeline job

With this commit v1.36 and v1.36-upstream-terraform of the Tectonic
builder images are pushed to quay.io/coreos/tectonic-builder. There
are no functional changes, but changes to the build order in the Dockerfile.

You can find the Jenkins job here.

mxinden · 2017-07-20T16:56:54Z

Forgot to do some code updates. Moving back to WIP.

mxinden · 2017-07-21T10:56:09Z

@s-urbaniak @cpanato Ready for review.

s-urbaniak · 2017-07-21T11:17:57Z

images/tectonic-builder/README.md

+
+## Upstream and CoreOS Terraform
+
+We are currently using a custom terraform version for our default CI pipeline


s/We/The Tectonic CI

s-urbaniak

no objections, just a wording nit.

cpanato · 2017-07-21T11:20:26Z

images/tectonic-builder/Dockerfile

 # docker tag <resulting-image-id> quay.io/coreos/tectonic-builder:<next-semver>
 # docker push quay.io/coreos/tectonic-builder:<next-semver>
+#
+# docker build --build-arg TERRAFORM_URL=<upstream terraform download url> -f builder/Dockerfile .


the path for the Dockerfile is not right

also dont need to use the -t quay.io/coreos/tectonic-builder:v1.33-upstream-terraform as described in the readme?

Its up to you if you build and then tag, or just tag during building via -t. But probably easier in two steps. Adjusted accordingly. Thanks.

cpanato

LGTM, just my comment need to check but it is a non blocker

mxinden · 2017-07-21T12:01:42Z

@s-urbaniak @cpanato Thanks for the review. Addressed all suggestions. Let me know if you see anything else.

mxinden · 2017-07-21T12:37:22Z

@squat @Quentin-M @estroz Feel free to merge if you have no objections.

squat

Everything looks good except for a small nit in documentation

squat · 2017-07-24T16:25:16Z

images/tectonic-builder/README.md

+
+The Tectonic CI is currently using a custom terraform version for the default
+pipeline (See https://github.com/coreos/tectonic-installer/pull/1247). As end
+users of Tectonic installer use upstream terraform we need to test with upstream


let's be consistent with the casing of Terraform. Either terraform or Terraform but we should not switch.

mxinden · 2017-07-26T17:04:00Z

@squat Thanks for the catch. Adjusted accordingly. Let me know if you have any other objections.

cpanato · 2017-08-02T13:01:48Z

@mxinden could you please rebase

mxinden · 2017-08-02T13:07:49Z

@cpanato Sorry for the delay and thanks for the reminder. Rebased. Let me know what you think.

cpanato

LGTM

mxinden · 2017-08-02T13:15:16Z

This will not turn green until #1558 is merged.

mxinden · 2017-08-04T11:13:19Z

The only breaking change of this PR is
def default_builder_image = 'quay.io/coreos/tectonic-builder:v1.36' in the Jenkinsfile, conformance script and release script. v1.36 vs 1.35 only differs in the docker build execution order.

In build number 13 everything is green except:

SmokeTest TerraForm: AWS
SmokeTest TerraForm: AWS (network policy)

Both of these stages succeeded in build number 15.

Instead of retriggering the tests until all are green on this minor change, I would prefer to merge, even though the current build is not fully green.

@squat If you feel comfortable with this, please merge.

alexsomesan · 2017-08-09T11:32:12Z

images/tectonic-builder/Dockerfile

 # docker push quay.io/coreos/tectonic-builder:<next-semver>
+#
+# docker build \
+# --build-arg TERRAFORM_URL=<upstream terraform download url> \


Is this the release bundle or the binary itself?

Nevermind, I saw it below 👍

alexsomesan · 2017-08-09T11:33:50Z

upstream-terraform-trigger.groovy

@@ -0,0 +1,3 @@
+node {
+  build job: 'tectonic-installer/master', parameters: [string(name: 'builder_image', value: 'quay.io/coreos/tectonic-builder:v1.36-upstream-terraform')] 


This is hard to comprehend on one line.
Any chance we could break it down into multiple lines?

We are currently using a custom terraform version for our default CI pipeline (See coreos#1247). As end users of Tectonic installer use upstream terraform we need to test with upstream terraform as well. We are building and publishing the Tectonic builder image both with CoreOS Terraform as well as upstream Terraform. This is done via docker `--build-arg` `TERRAFORM_URL`. CoreOS Terraform is used for normal PR and branch tests, upstream Terraform is used once per day on master. * Add `--build-arg` `TERRAFORM_URL` to Tectonic builder Docker file * Update documentation in tectonic-builder/README.md * Add upstream-terraform-trigger.groovy pipeline job With this commit v1.36 and v1.36-upstream-terraform of the Tectonic builder images are pushed to quay.io/coreos/tectonic-builder. There are no functional changes, but changes to the build order in the Dockerfile. You can find the Jenkins job [here](https://jenkins-tectonic-installer.prod.coreos.systems/job/upstream-terraform-trigger/).

mxinden · 2017-08-09T11:49:36Z

@alexsomesan Broke it down to multiple lines. Let me know if there is anything else?

alexsomesan

Looks good!
Let get this in.

Concerns were only related to documentation. They have been addressed. In addition Alex looked over all new changes.

These were added in 15394f8 (tests: Add upstream terraform trigger, 2017-07-21, coreos/tectonic-installer#1444), but we returned to upstream Terraform in all cases in 379fd19 (Bump Terraform to 0.10.7, 2017-10-04, coreos/tectonic-installer#2041). Remove the stale docs.

mxinden force-pushed the upstream-terraform-trigger branch from 139feae to 069f63f Compare July 20, 2017 10:05

mxinden changed the title ~~[WIP] tests: Add upstream terraform trigger~~ tests: Add upstream terraform trigger Jul 20, 2017

mxinden requested review from cpanato and s-urbaniak July 20, 2017 15:30

mxinden force-pushed the upstream-terraform-trigger branch 3 times, most recently from 5931e1a to 926bc28 Compare July 20, 2017 16:50

mxinden removed request for cpanato and s-urbaniak July 20, 2017 16:56

mxinden changed the title ~~tests: Add upstream terraform trigger~~ [WIP] tests: Add upstream terraform trigger Jul 20, 2017

mxinden force-pushed the upstream-terraform-trigger branch 2 times, most recently from 9b3ddec to 323b380 Compare July 21, 2017 07:13

mxinden changed the title ~~[WIP] tests: Add upstream terraform trigger~~ tests: Add upstream terraform trigger Jul 21, 2017

mxinden requested review from cpanato and s-urbaniak July 21, 2017 10:55

mxinden added area/ci area/testing needs review labels Jul 21, 2017

s-urbaniak reviewed Jul 21, 2017

View reviewed changes

s-urbaniak previously approved these changes Jul 21, 2017

View reviewed changes

cpanato reviewed Jul 21, 2017

View reviewed changes

cpanato previously approved these changes Jul 21, 2017

View reviewed changes

mxinden dismissed stale reviews from cpanato and s-urbaniak via 82a60f6 July 21, 2017 12:00

mxinden force-pushed the upstream-terraform-trigger branch from 323b380 to 82a60f6 Compare July 21, 2017 12:00

cpanato previously approved these changes Jul 21, 2017

View reviewed changes

s-urbaniak previously approved these changes Jul 21, 2017

View reviewed changes

squat previously requested changes Jul 24, 2017

View reviewed changes

mxinden dismissed stale reviews from s-urbaniak and cpanato via 7bc232b July 26, 2017 17:03

mxinden force-pushed the upstream-terraform-trigger branch from 82a60f6 to 7bc232b Compare July 26, 2017 17:03

mxinden force-pushed the upstream-terraform-trigger branch from 7bc232b to 044d20f Compare August 2, 2017 13:07

mxinden mentioned this pull request Aug 2, 2017

tests: make upstream-terraform-trigger fully reproducable #1559

Closed

cpanato previously approved these changes Aug 2, 2017

View reviewed changes

cpanato mentioned this pull request Aug 2, 2017

tests/ci: add upstream-terraform-trigger as jenkins script #1565

Merged

mxinden dismissed cpanato’s stale review via 4659a6c August 9, 2017 07:57

mxinden force-pushed the upstream-terraform-trigger branch from 044d20f to 4659a6c Compare August 9, 2017 07:57

alexsomesan reviewed Aug 9, 2017

View reviewed changes

mxinden force-pushed the upstream-terraform-trigger branch from 4659a6c to 15394f8 Compare August 9, 2017 11:41

alexsomesan approved these changes Aug 9, 2017

View reviewed changes

mxinden merged commit aa669ad into coreos:master Aug 9, 2017

wking mentioned this pull request Jul 9, 2018

images: Remove references to CoreOS Terraform openshift/installer#35

Merged


		## Upstream and CoreOS Terraform

		We are currently using a custom terraform version for our default CI pipeline

		@@ -0,0 +1,3 @@
		node {
		build job: 'tectonic-installer/master', parameters: [string(name: 'builder_image', value: 'quay.io/coreos/tectonic-builder:v1.36-upstream-terraform')]

Conversation

mxinden commented Jul 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mxinden commented Jul 20, 2017

Uh oh!

mxinden commented Jul 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

s-urbaniak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpanato left a comment

Choose a reason for hiding this comment

Uh oh!

mxinden commented Jul 21, 2017

Uh oh!

mxinden commented Jul 21, 2017

Uh oh!

squat left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mxinden commented Jul 26, 2017

Uh oh!

cpanato commented Aug 2, 2017

Uh oh!

mxinden commented Aug 2, 2017

Uh oh!

cpanato left a comment

Choose a reason for hiding this comment

Uh oh!

mxinden commented Aug 2, 2017

Uh oh!

mxinden commented Aug 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mxinden commented Aug 9, 2017

Uh oh!

alexsomesan left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mxinden commented Jul 20, 2017 •

edited

Loading