Remove bazel from test process, use 'terraform fmt' 'go test' by sallyom · Pull Request #97 · openshift/installer

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

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

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

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

Details

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

Conversation

sallyom commented Aug 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coreosbot commented Aug 1, 2018

Uh oh!

wking 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!

Choose a reason for hiding this comment

Uh oh!

wking Aug 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

yifan-gu commented Aug 9, 2018

Uh oh!

openshift-ci-robot commented Aug 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

sallyom commented Aug 1, 2018 •

edited

Loading

wking Aug 3, 2018 •

edited

Loading