Prow: Add shell check to openshift/installer #1131
Conversation
openshift-ci-robot
added
size/S
|
/assign @bbguimaraes |
cluster/ci/config/prow/config.yaml
Outdated
| - sh | ||
| args: | ||
| - -c | ||
| - 'for file in $(find ./ -type f -name "*.sh"); |
There was a problem hiding this comment.
This is fine but generally you want to add this as a script in the repository so that developers can run it locally, too
There was a problem hiding this comment.
@stevekuznetsov @alejovicu we can include shellcheck|terraform-lint|terraform-fmt|go unit tests as targets in a Makefile in the installer repo? then combine them all here with a single make cmd? (eventually, we're replacing the bazel build with Make, too, AFAIK. We can start with a simple Makefile for testing to replace the travis cmds, then add on in near future?
There was a problem hiding this comment.
we can include shellcheck|terraform-lint|terraform-fmt|go unit tests as targets in a Makefile in the installer repo? then combine them all here with a single make cmd?
The only downside I see to using a single make call here would be granular retesting. All of these are fast, so I'm in favor of lumping them all into a single Prow test, but I wouldn't want something heavy (like e2e tests) lumped in with them ;).
There was a problem hiding this comment.
Actually, another issue with lumping them together would be errors in one test masking issues in another. My preferred approach to that would be to use --keep-going, so a failure in one subtest (e.g. Terraform formatting) wouldn't mask errors in another (e.g. Go unit tests).
There was a problem hiding this comment.
@wking we had a long discussion on this :) I think @alejovicu @stevekuznetsov and I are all on the same page with this now.. we'll def want to have separate targets.. like 'make tflint, make cli_unit, etc' and we'll have a hack directory (following convention of other openshift/repos) where we'll keep the simple scripts, like 'tflint.sh', etc
EDIT: we'll simply call the small test scripts in prow directly, rather than introduce a Makefile for this, for now
cluster/ci/config/prow/config.yaml
Outdated
| @@ -3209,6 +3209,33 @@ presubmits: | |||
| - --template=/usr/local/e2e-aws | |||
| - --target=e2e-aws | |||
|
|
|||
| - name: shell-check | |||
| trigger: "(?m)^/shell-check" | |||
| rerun_command: "/shell-check" | |||
There was a problem hiding this comment.
As in the other PR, convention is to use /test <name> and have context ci/prow/<name>
|
/ok-to-test |
openshift-ci-robot
removed
the
needs-ok-to-test
|
Please do not merge until this openshift/installer#104 is merged |
It landed this morning, so we should be good to go here. |
cluster/ci/config/prow/config.yaml
Outdated
| @@ -3230,6 +3230,25 @@ presubmits: | |||
| args: | |||
| - ./hack/yaml-lint.sh | |||
|
|
|||
| - name: shellcheck | |||
There was a problem hiding this comment.
sorry, final nit -- this is usually ci-pull-$org-$repo-$name
|
/lgtm |
|
@alejovicu: Updated the
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
These have moved into Prow presubmit jobs: * Terraform lint: openshift/release@82e00346 (Prow: Add Terraform Lint to openshift/installer, 2018-08-06, openshift/release#1124). * YAML lint: openshift/release@457be2cd (Added prow yaml-lint job description for installer repo, 2018-08-02, openshift/release#1138). * ShellCheck: openshift/release@e12a7a06 (Prow: Add shellcheck to openshift/installer, 2018-08-08, openshift/release#1131). * Terraform format: openshift/release@f67e06e4 (openshift installer: add terraform fmt, 2018-08-04, openshift/release#1152). * Go vet: openshift/release@71afdcca (Added go-vet prow job, 2018-08-14, openshift/release#1181). * Building the tarball: openshift/release@42a5a0d0 (add openshift/installer 'bazel build tarball' test to prow, 2018-08-13, openshift/release#1178).
Add Shell check to openshift/installer. (CORS-746)