-
-
Notifications
You must be signed in to change notification settings - Fork 455
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
add: Allow config and pre-processed configuration to leverage environment variables ($K3D_CONFIG env var) #1446
base: main
Are you sure you want to change the base?
Conversation
cfd98a3
to
bef7bf8
Compare
Hey @dprunier , thanks for this PR! For some reason, I have no way of triggering the test suite on your PR - but it seems to fail on my machine. |
bef7bf8
to
5f74fc0
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.
The config test is still failing for me - it seems like it doesn't consider the config passed in via --config
at all anymore
EDIT: Since you removed the var, where we stored the --config
flag value before, we don't have access to the value anymore as ppViper doesn't know about that flag. Here's a patch (renamed as txt to allow that in GitHub) that would solve this for clusterCreate.
Hey @iwilltry42. sorry for the long delay, I was out for a while. Started to look at this today, it looks like the patch you attached is exactly the current diff, am I missing something ? Thx. |
@dprunier whoops, yeah you're right, I attached the wrong patch. Here's the correct one: |
6f3de1c
to
a824179
Compare
@iwilltry42 nevermind, I think I found the issue... it was a stupid oversight on my end (and clearly lack of proper testing :D) The test is now passing but raised a concern with the behavior of the delete command: currently, when a configuration file is passed, it is assumed that it must contain a cluster name (as this is the only real reason to pass a config file in the first place). The problem is that in my case, I have a system wide K3D_CONFIG environment variable set (to set a few defaults for my own use) but it doesn't contain a name. In such case, the delete command never works and always fails with I believe there would be an easy fix: when a configuration file is given and contains a name, it is used, otherwise it behaves as if no configuration file was given. I can do this change real quick but:
|
I'm fine with that slight change in behavior, which means that if a) config file does not contain a name and b) Feel free to add it to this PR and I'll account for it in the changelog and release notes 👍 |
de42f1d
to
fff51e9
Compare
Cool. I've added it with fff51e9. The behavior is pretty simple: instead of failing as soon as one specifies a configuration file with With that change, the E2E test |
I think that |
Indeed. To make things clear, given the
The current (
With the implementation from this PR, it looks like this (the difference is the second line):
And I believe what you're suggesting is this (the difference is the third line):
I can do this change if you agree. It is worth mentioning that it would make it more consistent with Let me know. |
What
Currently, there are a few command line arguments for
cluster create
andcluster delete
that cannot be specified using environment variables. This PR tries to address this by leveragingviper
, which is already being used byk3d
.Why
I'd like to set a default configuration for
k3d
, and the easiest way would be to haveK3D_CONFIG=${HOME}/.k3d/config.yaml
in my shell environment.Another approach could have been to use
viper
configuration loading mechanism (which has a mechanism to locate configuration files from various locations) but it would have been a more involving change.Implications
I don't think there are any, as the change is relatively minor and backward compatible.