Remove bazel from test process, use 'terraform fmt' 'go test' #97

sallyom · 2018-08-01T23:15:12Z

CORS-749, CORS-750

prow config for 'go test', 'terraform fmt' here: openshift/release#1152

Chipping away at the .travis.yml tasks, this PR moves Terraform fmt and Installer Unit Tests to Prow

coreosbot · 2018-08-01T23:15:14Z

Can one of the admins verify this patch?

wking

I'd also like to update Travis to use the new scripts. I think that makes the motivation clearer, and also helps exercise the scripts before Prow picks them up. Not a big deal either way though.

wking · 2018-08-03T04:54:20Z

hack/cli_units.sh

+  if [ "$FAILED" != "" ]; then
+    exit 1
+  fi
+  echo cli_units passed


I think we can simplify this to:

if [ "$IS_CONTAINER" != "" ]; then go test ./installer/... else docker ... fi

If Prow needs success stdout, it can ./hack/cli_units.sh && echo 'cli_units passed'. See the similar discussion here.

wking · 2018-08-03T05:11:41Z

hack/terraform_fmt.sh

+bazel --output_base=.cache build tarball
+tar -zxf bazel-bin/tectonic-dev.tar.gz
+cd tectonic-dev || exit 1
+PATH=$(pwd)/installer:$PATH


This seems like a lot of work to install Terraform, when the end result will be downloading it. Maybe there's a stock Terraform image we can use (the tflint image we already use?)?

ah that would be great.. looking into it, yes, I know this is silly.. but I'm just getting my bearings with this codebase atm.. thanks

wking · 2018-08-03T14:11:55Z

hack/gotest.sh

@@ -0,0 +1,14 @@
+#!/bin/sh
+
+CGO_ENABLED=0


You may need to export this or move it to the go test ... line:

$ ABC=1 $ env | grep ABC $ ABC=1 env | grep ABC ABC=1 $ export ABC=1 $ env | grep ABC ABC=1

whoops, yes thanks

wking · 2018-08-03T14:17:18Z

hack/gotest.sh

+  cd installer/pkg
+  PKGS=$(go list ./...)
+  for p in $PKGS; do
+    go test "$p" -cover


We want to fail this script if any of these fail, and a for loop makes that harder. I think you can just do:

if [ "$IS_CONTAINER" != "" ]; then CGO_ENABLED=0 go test -cover ./installer/... else docker ... fi

yes, you are right. fixed

wking · 2018-08-07T12:58:46Z

.travis.yml

+    - script: ./hack/tf-fmt.sh
+      name: Terraform format
+    - script: ./hack/gotest.sh
+      name: go test


Shouldn't these replace the "Terraform tests" and "Installer unit tests" entries below?

yes they should, and now they do, thanks

CORS-749, CORS-750

yifan-gu · 2018-08-09T00:31:33Z

/lgtm

openshift-ci-robot · 2018-08-09T00:31:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sallyom, yifan-gu

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [yifan-gu]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

The script is from b8a9bbc (Remove bazel from test process, 2018-08-01, openshift#97), as part of moving tests from Travis to Prow. But Prow is already running our Go unit tests, via pull-ci-origin-installer-unit [1] with this config [2]. It's had that setup since openshift/release@eb11aa8b (Bump config to take advantage of canonical setup, 2018-06-06, openshift/release#921), so we don't need to replicate it here. [1]: https://github.com/openshift/release/blob/6f623b15854ae91203243af22d7a259ab85acd86/ci-operator/jobs/openshift/installer/openshift-installer-presubmits.yaml#L3-L28 [2]: https://github.com/openshift/release/blob/6f623b15854ae91203243af22d7a259ab85acd86/ci-operator/config/openshift/installer/master.json#L44

Eric points out potential issues with relabeling /tmp [1], which is shared by several system-level consumers. For the Bazel script, the /tmp mount is just since c483f59 (Move bazel build tarball test to prow, 2018-08-08, openshift#117), so we can drop it to return to our previous approach. The Terraform container seems to run fine without /tmp as well, although there's no clear history to point to on this front because we used to use Bazel for this. See b8a9bbc (Remove bazel from test process, 2018-08-01, openshift#97). [1]: openshift#174 (comment)

Merge 20220112

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 1, 2018

openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 1, 2018

wking reviewed Aug 3, 2018

View reviewed changes

sallyom force-pushed the remove-bazel-from-tests branch 2 times, most recently from 68ad409 to b772f38 Compare August 3, 2018 13:29

openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 3, 2018

sallyom force-pushed the remove-bazel-from-tests branch from b772f38 to 970c45d Compare August 3, 2018 13:32

wking reviewed Aug 3, 2018

View reviewed changes

sallyom force-pushed the remove-bazel-from-tests branch 2 times, most recently from bcc9653 to 6e1b638 Compare August 4, 2018 19:42

openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 4, 2018

sallyom force-pushed the remove-bazel-from-tests branch from 6e1b638 to 815b44b Compare August 4, 2018 19:53

sallyom changed the title ~~WIP: Remove bazel from test process~~ Remove bazel from test process, use 'terraform fmt' 'go test' Aug 4, 2018

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 4, 2018

sallyom mentioned this pull request Aug 4, 2018

openshift installer: add terraform fmt openshift/release#1152

Merged

sallyom force-pushed the remove-bazel-from-tests branch 3 times, most recently from e3dae39 to 57b9fe5 Compare August 6, 2018 20:44

wking reviewed Aug 7, 2018

View reviewed changes

sallyom force-pushed the remove-bazel-from-tests branch 5 times, most recently from 051455d to 0073869 Compare August 8, 2018 20:00

Remove bazel from test process

b8a9bbc

CORS-749, CORS-750

sallyom force-pushed the remove-bazel-from-tests branch from 0073869 to b8a9bbc Compare August 8, 2018 20:07

openshift-ci-robot assigned yifan-gu Aug 9, 2018

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 9, 2018

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 9, 2018

openshift-merge-robot merged commit 745b56c into openshift:master Aug 9, 2018

wking mentioned this pull request Aug 14, 2018

.travis.yml: Prune moved-to-Prow tf-lint, etc. #123

Merged

wking mentioned this pull request Aug 15, 2018

hack/gotest.sh: Drop unnecessary script #135

Merged

miyamotoh pushed a commit to miyamotoh/installer that referenced this pull request Jan 20, 2022

Merge pull request openshift#97 from clnperez/merge-20220112

480b2ae

Merge 20220112

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove bazel from test process, use 'terraform fmt' 'go test' #97

Remove bazel from test process, use 'terraform fmt' 'go test' #97

sallyom commented Aug 1, 2018 •

edited

Loading

coreosbot commented Aug 1, 2018

wking left a comment

wking Aug 3, 2018

wking Aug 3, 2018

sallyom Aug 3, 2018

wking Aug 3, 2018

sallyom Aug 6, 2018

wking Aug 3, 2018 •

edited

Loading

sallyom Aug 6, 2018

wking Aug 7, 2018

sallyom Aug 7, 2018

yifan-gu commented Aug 9, 2018

openshift-ci-robot commented Aug 9, 2018

Remove bazel from test process, use 'terraform fmt' 'go test' #97

Remove bazel from test process, use 'terraform fmt' 'go test' #97

Conversation

sallyom commented Aug 1, 2018 • edited Loading

coreosbot commented Aug 1, 2018

wking left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wking Aug 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yifan-gu commented Aug 9, 2018

openshift-ci-robot commented Aug 9, 2018

sallyom commented Aug 1, 2018 •

edited

Loading

wking Aug 3, 2018 •

edited

Loading