Unset certain environment variables before testing #4595
Conversation
k8s-ci-robot
added
needs-ok-to-test
|
/ok-to-test |
k8s-ci-robot
removed
the
needs-ok-to-test
|
/assign |
Makefile
Outdated
| @@ -210,6 +210,7 @@ hooks: # Install Git hooks | |||
| cp hack/pre-commit.sh .git/hooks/pre-commit | |||
|
|
|||
| .PHONY: test | |||
| unexport PROTOKUBE_IMAGE NODEUP_URL KOPS_BASE_URL DNSCONTROLLER_IMAGE KOPS_RUN_OBSOLETE_VERSION ENV_VAR_CNI_VERSION_URL AWS_REGION AWS_ACCESS_KEY_ID AWS_SECRET_ACCESS_KEY AWS_SESSION_TOKEN AWS_PROFILE | |||
There was a problem hiding this comment.
What's the scope of unexport here? Will it only apply to the test recipe, or every recipe below it as well?
There was a problem hiding this comment.
@rifelpet it appears that unexport has global scope, so we should put it at the top of the file for clarity.
|
thanks @rifelpet I'll do some testing with this shortly. |
|
Unexport causes all make targets to ignore the specified environment variables. I don't think any of those included in this PR are needed at build time. But would like a second pair of eyes from @chrislovecnm or @justinsb |
rifelpet
force-pushed
the
2657-unset-env-vars
branch
from
3fcfe9f to
3687787
Compare
|
@robinpercy Moved the unexport towards the top of the Makefile |
|
@robinpercy I think we want to unexport all those variables and I think more. As long as make is not crazy and unsets the variable in my current shell environment, I think we are good. @rifelpet did you get all those pesky variables? If I can read today, which is often debatable, I think we are missing STATE_STORE. Another issue is to document all these pesky rabbits, aka ENV VARs. |
|
Oh and we just merged KOPS_CLUSTER_NAME I think ... |
rifelpet
force-pushed
the
2657-unset-env-vars
branch
from
3687787 to
1175761
Compare
| @@ -45,6 +45,11 @@ API_OPTIONS?= | |||
| # See http://stackoverflow.com/questions/18136918/how-to-get-current-relative-directory-of-your-makefile | |||
| MAKEDIR:=$(strip $(shell dirname "$(realpath $(lastword $(MAKEFILE_LIST)))")) | |||
|
|
|||
| # Unexport environment variables that can affect tests and are not used in builds | |||
There was a problem hiding this comment.
@rifelpet Nit: could we sort these in alphabetical order? It will make future management of the list less error-prone.
There was a problem hiding this comment.
Yeah, I have them categorized by kops-specific, aws-specific, and other cloud providers, but ill switch it to entirely alphabetical
rifelpet
force-pushed
the
2657-unset-env-vars
branch
from
1175761 to
4772814
Compare
|
Thanks @rifelpet! @chrislovecnm were there any other variables you wanted to see in here? |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chrislovecnm, rifelpet 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 |
k8s-ci-robot
added
the
approved
Fixes #2657 and #4339