openshift-installer-presubmits: Run go-lint on cmd/... and pkg/... #1289
Conversation
openshift-ci-robot
added
the
size/XS
openshift-ci-robot
added
the
approved
wking
force-pushed
the
installer-lint-root-pkg-directory
branch
from
707920d to
a69bc01
Compare
stevekuznetsov
left a comment
stevekuznetsov
left a comment
There was a problem hiding this comment.
Minor comments - other approaches may require fewer tweaks in the future.
There was a problem hiding this comment.
Why not
./...?
We are currently leaning on Go's test framework for smoke tests. We may drop those soon in favor of more e2e-aws coverage (#1271 is groundwork for this expansion), but until then we need to keep this job out of tests/smoke. A white-list like this is one way to do that.
There may also be blacklist approaches, but I think we'd need to include find in our invocation if we wanted to do that. Or maybe leaning on your go list suggestion?
There was a problem hiding this comment.
Why not go list -f '{{ .Dir }}' ./... ?
There was a problem hiding this comment.
Why not
go list -f '{{ .Dir }}' ./...?
Something like that could work, although:
$ sudo hack/go-lint.sh $(go list -f '{{ .Dir }}' ./...)
import "/home/trking/.local/lib/go/src/github.com/openshift/installer/cmd/openshift-install": cannot import absolute path
...It works with .ImportPath:
$ sudo hack/go-lint.sh $(go list -f '{{ .ImportPath }}' ./...)How do I encode that in YAML here? Something like:
- image: quay.io/coreos/golang-testing
env:
- name: IS_CONTAINER
value: "TRUE"
command:
- sh
args:
- -c
- ./hack/go-lint.sh $(go list -f '{{ .ImportPath }}' ./...)There was a problem hiding this comment.
I suspect that PodSpec would work, yes
There was a problem hiding this comment.
I suspect that PodSpec would work, yes
Updated with a69bc01 -> aaa039c.
wking
force-pushed
the
installer-lint-root-pkg-directory
branch
from
a69bc01 to
aaa039c
Compare
|
/lgtm |
Cover ./pkg/... (and, for go-lint, ./cmd/...). These directories are new with openshift/installer@4e92db8 (Merge pull request openshift#120 from staebler/asset_graph_engine, 2018-08-24, openshift/installer#120). There's no particular reason to not run the Go unit tests on ./cmd/... But we're unlikely to have unit tests there, and the old tests restricted themselves to ./installer/pkg/..., so I've stuck with that pattern here. Steve recommended using 'go list ...' to run the lint commands [1]. We just want something that will give us our Go directories without dropping down into the vendor directories. [1]: openshift#1289 (comment)
wking
force-pushed
the
installer-lint-root-pkg-directory
branch
from
aaa039c to
8f82768
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: stevekuznetsov, wking The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@wking: Updated the following 2 configmaps:
DetailsIn 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. |
* Fix path to .openshift_install_state.json Signed-off-by: Christoph Stäbler <[email protected]> * Add help where xinetd config parameters can be found Signed-off-by: Christoph Stäbler <[email protected]>
4 participants
Cover these directories, which are new with openshift/installer#120.