-
Notifications
You must be signed in to change notification settings - Fork 255
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
be a bit more verbose on a required KUBECONFIG env var #404
be a bit more verbose on a required KUBECONFIG env var #404
Conversation
/assign @marquiz |
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.
Nit about the commit message subject: I'd suggest to start with a "context" (i.e. Makefile:
) in order to better describe where this change applies to, i.e. have something like
Makefile: require KUBECONFIG to be specified for e2e-test target
Makefile
Outdated
@@ -113,7 +113,8 @@ test: | |||
$(GO_CMD) test ./cmd/... ./pkg/... | |||
|
|||
e2e-test: | |||
$(GO_CMD) test -v ./test/e2e/ -args -nfd.repo=$(IMAGE_REPO) -nfd.tag=$(IMAGE_TAG_NAME) -kubeconfig=$(KUBECONFIG) -nfd.e2e-config=$(E2E_TEST_CONFIG) -ginkgo.focus="\[NFD\]" | |||
@if [ -z ${KUBECONFIG} ]; then echo "[ERR] KUBECONFIG missing, must be defined" && exit 1; fi |
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.
nit: use ;
instead of &&
between echo and exit
88d0789
to
defb574
Compare
Also just noted, I got to do the 404 pun !!! PR not found LOL |
Makefile
Outdated
@@ -113,7 +113,8 @@ test: | |||
$(GO_CMD) test ./cmd/... ./pkg/... | |||
|
|||
e2e-test: | |||
$(GO_CMD) test -v ./test/e2e/ -args -nfd.repo=$(IMAGE_REPO) -nfd.tag=$(IMAGE_TAG_NAME) -kubeconfig=$(KUBECONFIG) -nfd.e2e-config=$(E2E_TEST_CONFIG) -ginkgo.focus="\[NFD\]" | |||
@if [ -z ${KUBECONFIG} ]; then echo "[ERR] KUBECONFIG missing, must be defined"; exit 1; fi | |||
$(GO_CMD) test -v ./test/e2e/ -args -nfd.repo=$(IMAGE_REPO) -nfd.tag=$(IMAGE_TAG_NAME) -kubeconfig=$(KUBECONFIG) -nfd.e2e-config=$(E2E_TEST_CONFIG) -ginkgo.focus="\[NFD\]"; \ |
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.
Ach, I forgot this from the previous review: you shouldn't need to change this line (this ; \
is unnecessary)
Otherwise looks good
5394f02
to
3b2b312
Compare
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.
Some nits.
Plus drop bin/nfd-master
and bin/nfd-worker
from the patch(!) Maybe we should add bin/
to gitignore...
Makefile
Outdated
@@ -113,7 +113,8 @@ test: | |||
$(GO_CMD) test ./cmd/... ./pkg/... | |||
|
|||
e2e-test: | |||
$(GO_CMD) test -v ./test/e2e/ -args -nfd.repo=$(IMAGE_REPO) -nfd.tag=$(IMAGE_TAG_NAME) -kubeconfig=$(KUBECONFIG) -nfd.e2e-config=$(E2E_TEST_CONFIG) -ginkgo.focus="\[NFD\]" | |||
@if [ -z ${KUBECONFIG} ]; then echo "[ERR] KUBECONFIG missing, must be defined"; exit 1; fi |
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.
nit: double-space before exit
Makefile
Outdated
@@ -113,7 +113,8 @@ test: | |||
$(GO_CMD) test ./cmd/... ./pkg/... | |||
|
|||
e2e-test: | |||
$(GO_CMD) test -v ./test/e2e/ -args -nfd.repo=$(IMAGE_REPO) -nfd.tag=$(IMAGE_TAG_NAME) -kubeconfig=$(KUBECONFIG) -nfd.e2e-config=$(E2E_TEST_CONFIG) -ginkgo.focus="\[NFD\]" | |||
@if [ -z ${KUBECONFIG} ]; then echo "[ERR] KUBECONFIG missing, must be defined"; exit 1; fi | |||
$(GO_CMD) test -v ./test/e2e/ -args -nfd.repo=$(IMAGE_REPO) -nfd.tag=$(IMAGE_TAG_NAME) -kubeconfig=$(KUBECONFIG) -nfd.e2e-config=$(E2E_TEST_CONFIG) -ginkgo.focus="\[NFD\]" |
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.
nit: remove trailing whitespace from the end of the line
|
3b2b312
to
76c1117
Compare
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.
Please squash into one oneliner patch, and, rebase on top of latest master. Gitignore was already updated in #405 so no need to touch that
cb69c92
to
950675a
Compare
Done |
This Patch adds a check on make target e2e-test, to check if the KUBECONFIG env var is set before running the test suite. if not pressent return a valid error message. Signed-off-by: Carlos Eduardo Arango Gutierrez <carangog@redhat.com>
950675a
to
30e6446
Compare
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.
Thanks @ArangoGutierrez
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ArangoGutierrez, marquiz 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:
Approvers can indicate their approval by writing |
throw an error message before running
e2e-test
ifKUBECONFIG
is not definedSigned-off-by: Carlos Eduardo Arango Gutierrez carangog@redhat.com