-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
created structure for whitelisting directories for govet shadow testing #6509
created structure for whitelisting directories for govet shadow testing #6509
Conversation
@@ -5,7 +5,7 @@ set -o pipefail | |||
|
|||
GO_VERSION=($(go version)) | |||
|
|||
if [[ -z $(echo "${GO_VERSION[2]}" | grep -E 'go1.4') ]]; then | |||
if [[ -z $(echo "${GO_VERSION[2]}" | grep -E 'go1.5') ]]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
flags have changed for go tool vet
and I think we can migrate testing to 1.5.*
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Jenkins doesn't run on 1.5 yet, so if you want this actually enforced, it has to be 1.4 as well
293ec29
to
04aaceb
Compare
./pkg/user | ||
./pkg/util | ||
./pkg/version | ||
./plugins |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather blacklist so we can delete exceptions as we clean them up and new packages will get enforced.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
ff5ef6d
to
5578a20
Compare
[test] |
5578a20
to
0dfe998
Compare
@liggitt made changes to programatically generate |
@@ -23,13 +23,69 @@ FAILURE=false | |||
test_dirs=$(find_files | cut -d '/' -f 1-2 | sort -u) | |||
for test_dir in $test_dirs | |||
do | |||
go tool vet -shadow=false $test_dir | |||
go tool vet -all $test_dir |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should still be -shadow=false, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no. the flags to go vet
changed with the latest release.
-all
== all, except shadow
do | ||
# use `grep` failure to determine that a directory is not in the blacklist | ||
if ! echo "${DIR_BLACKLIST}" | grep -q "${test_dir}"; then | ||
go tool vet -shadow -shadowstrict $test_dir |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does vet
recurse subpackages? If not, then you need ALL_DIRS to be all packages, and DIR_BLACKLIST to filter out by prefix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
vet
recurses directories, which is what we are passing it
0dfe998
to
b2e1f7f
Compare
flake on
#6065 |
more flakes with webdriver here:
#6533 |
# use `grep` failure to determine that a directory is not in the blacklist | ||
if ! echo "${DIR_BLACKLIST}" | grep -q "${test_dir}"; then | ||
go tool vet -shadow -shadowstrict $test_dir | ||
if [ "$?" -ne 0 ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems odd to compare a string and a number... this probably works, but make both sides one or the other
nit, LGTM |
b2e1f7f
to
e2ffd47
Compare
@liggitt addressed comment |
e2ffd47
to
27df269
Compare
Evaluated for origin test up to 27df269 |
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/8181/) |
@liggitt this is ready |
[merge] |
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/4676/) (Image: devenv-rhel7_3193) |
Evaluated for origin merge up to 27df269 |
Merged by openshift-bot
@liggitt PTAL
follow-up pulls expand the whitelist
grep
and fix shadowed variables