Skip to content
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

Introduce KUBECACHEDIR environment variable to override default discovery cache dir #109479

Merged
merged 3 commits into from Jun 29, 2022

Conversation

ardaguclu
Copy link
Member

@ardaguclu ardaguclu commented Apr 14, 2022

What type of PR is this?

/kind feature

What this PR does / why we need it:

This PR introduces new environment variable, namely KUBECACHEDIR. KUBECACHEDIR
is used to override default discovery cache directory for all commands(whose default value
is $HOME/.kube/cache).

--cache-dir flag per command has higher precedence than KUBECACHEDIR and default
directory path.

Which issue(s) this PR fixes:

Fixes kubernetes/kubectl#1184

Does this PR introduce a user-facing change?

Introduce new KUBECACHEDIR environment variable to override default discovery cache directory which is $HOME/.kube/cache

…very cache dir

This PR introduces new environment variable, namely `KUBECACHEDIR`. `KUBECACHEDIR`
is used to override default discovery cache directory for all commands(whose default value
is $HOME/.kube/cache).

`--cache-dir` flag per command has higher precedence than `KUBECACHEDIR` and default
directory path.
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Apr 14, 2022
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Apr 14, 2022

@ardaguclu: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

@k8s-ci-robot k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Apr 14, 2022
@k8s-ci-robot k8s-ci-robot requested review from eddiezane and seans3 Apr 14, 2022
@k8s-ci-robot k8s-ci-robot added sig/cli Categorizes an issue or PR as relevant to SIG CLI. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Apr 14, 2022
@ardaguclu
Copy link
Member Author

ardaguclu commented Apr 14, 2022

/cc @soltysh @brianpursley

@ardaguclu
Copy link
Member Author

ardaguclu commented Apr 14, 2022

/test pull-kubernetes-unit

@@ -420,7 +426,7 @@ func NewConfigFlags(usePersistentConfig bool) *ConfigFlags {
Timeout: utilpointer.String("0"),
KubeConfig: utilpointer.String(""),

CacheDir: utilpointer.String(defaultCacheDir),
CacheDir: utilpointer.String(""),
Copy link
Member

@brianpursley brianpursley Apr 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This potentially affects someone who is using NewConfigFlags(). Previously, it would return a ConfigFlags with CacheDir already populated with the default cache directory. Now it will return an empty CacheDir. This could be a breaking change for someone if they have built a client that uses this to know where the cache directory will be located.

There really isn't any way for a caller to know what the cache directory is going to be otherwise.

I wonder if it would be better to just change how defaultCacheDir is determined and leave everything else as-is?

So instead of this:

var (
	defaultCacheDir = filepath.Join(homedir.HomeDir(), ".kube", "cache")
)

Can you set defaultCacheDir conditionally if the environment variable is set, and using the current default value otherwise? Maybe using an init() func or something to do that.

Then toDiscoveryClient() and NewConfigFlags() don't need to change at all and everything should work exactly the same as it has been, just with the environment variable option.

Thoughts on this?

Copy link
Member Author

@ardaguclu ardaguclu Apr 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is really a good point @brianpursley and indeed it breaks the backward compatibility.

But for your suggestion, after setting KUBECACHEDIR in init function, there might be a risk to not using latest KUBECACHEDIR in case of updates?

Maybe we can leave defaultCacheDir as is and we can just use cache-dir if it is not equal to defaultCacheDir.

I mean something like that;

cacheDir := defaultCacheDir
if kcd := os.Getenv("KUBECACHEDIR"); kcd != "" {
cacheDir = kcd
}

// retrieve a user-provided value for the "cache-dir"
// override httpCacheDir and discoveryCacheDir if user-value is given.
// user-provided value has higher precedence than default
// and KUBECACHEDIR environment variable.
if f.CacheDir != nil && *f.CacheDir != "" && *f.CacheDir !=  defaultCacheDir {
cacheDir = *f.CacheDir
}

Only problem in here is user can not use defaultCacheDir by passing --cache-dir if KUBECACHEDIR is set but I think, this is too edge case?

Copy link
Member

@brianpursley brianpursley May 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is OK to change, it just needs to be set in all the same places it is currently.

What about something like this?...

Remove the defaultCacheDir variable and add a function that can return the default cache dir, something like this:

func getDefaultCacheDir() string {
	if kcd := os.Getenv("KUBECACHEDIR"); kcd != "" {
		return kcd
	}
	return filepath.Join(homedir.HomeDir(), ".kube", "cache")
}

Then you can replace defaultCacheDir with getDefaultCacheDir() which will follow the rules of looking at the KUBECACHEDIR environment variable first, then constructing it from the homedir if the environment variable is not set.

Copy link
Member Author

@ardaguclu ardaguclu May 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brianpursley thanks your the suggestion. I agree, this is better solution and changed this PR that way.

@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 29, 2022
@ardaguclu
Copy link
Member Author

ardaguclu commented Apr 29, 2022

/test pull-kubernetes-unit

1 similar comment
@ardaguclu
Copy link
Member Author

ardaguclu commented May 1, 2022

/test pull-kubernetes-unit

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels May 30, 2022
@brianpursley
Copy link
Member

brianpursley commented Jun 16, 2022

/priority backlog

@k8s-ci-robot k8s-ci-robot added priority/backlog Higher priority than priority/awaiting-more-evidence. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jun 16, 2022
Copy link
Member

@brianpursley brianpursley left a comment

/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 29, 2022
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Jun 29, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ardaguclu, brianpursley

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 29, 2022
@k8s-ci-robot k8s-ci-robot merged commit b2ed6ca into kubernetes:master Jun 29, 2022
14 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.25 milestone Jun 29, 2022
@ardaguclu ardaguclu deleted the new-kubecachedir-env branch Jun 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. priority/backlog Higher priority than priority/awaiting-more-evidence. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cli Categorizes an issue or PR as relevant to SIG CLI. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make cache-dir configurable wih env variable
3 participants