add: Allow config and pre-processed configuration to leverage environment variables ($K3D_CONFIG env var)

[dprunier]

What

Currently, there are a few command line arguments for cluster create and cluster delete that cannot be specified using environment variables. This PR tries to address this by leveraging viper, which is already being used by k3d.

Why

I'd like to set a default configuration for k3d, and the easiest way would be to have K3D_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.

[iwilltry42]

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.
Can you please give it a try?
It's good enough to only run the simple config file test: make e2e -e E2E_FAIL_FAST=1 -e E2E_LOG_LEVEL=debug -e E2E_INCLUDE="test_config_file"

[iwilltry42]

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.

[dprunier]

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.

[iwilltry42]

@dprunier whoops, yeah you're right, I attached the wrong patch. Here's the correct one:
0001-fix-config.patch.txt

[dprunier]

@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 failed to delete cluster via config file: no name in config file.

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:

• would you be ok with this change of behavior ?
• if so, should it be in that PR or another one ?

[iwilltry42]

I'm fine with that slight change in behavior, which means that if a) config file does not contain a name and b) --name is unset, we default to the default cluster name k3s-default, right?

Feel free to add it to this PR and I'll account for it in the changelog and release notes 👍

[dprunier]

Cool. I've added it with fff51e9. The behavior is pretty simple: instead of failing as soon as one specifies a configuration file with --config along with some names or --all as it used to, it is only doing so if there is a name in the configuration file. If there isn't any name in the configuration file, it is just ignored (as if none was passed with --config). That's probably the closest from the existing behavior, let me know if it works for you.

With that change, the E2E test test_config_file now passes.

[iwilltry42]

it is only doing so if there is a name in the configuration file

I think that --name and --all should just always take precedence over whatever there is in the config file.
I.e. a system-wide default config file that you have for example, could provide the default cluster name (like overriding k3s-default) - that would not work with the latest change, right?

[dprunier]

that would not work with the latest change, right?

Indeed. To make things clear, given the cluster delete syntax:

k3d cluster delete [--config FILE] [NAME [NAME ...] |--all] (Note: since this PR configuration file is specified using either --config or $K3D_CONFIG)

The current (main) behavior is:

	neither NAME nor --all specified	NAME specified	--all specified	both NAME and --all specified
no configuration	k3s-default	NAME	existing cluster names	existing cluster names, NAME ignored
with configuration file, and it does not contain a name	fail	fail	fail	fail
with configuration file and it contains a name	name from configuration file	fail	fail	fail

With the implementation from this PR, it looks like this (the difference is the second line):

	neither NAME nor --all specified	NAME specified	--all specified	both NAME and --all specified
no configuration	k3s-default	NAME	existing cluster names	existing cluster names (NAME ignored)
with configuration file, and it does not contain a name	k3s-default	NAME	existing cluster names	existing cluster names (NAME ignored)
with configuration file and it contains a name	name from configuration file	fail	fail	fail

And I believe what you're suggesting is this (the difference is the third line):

	neither NAME nor --all specified	NAME specified	--all specified	both NAME and --all specified
no configuration	k3s-default	NAME	existing cluster names	existing cluster names (NAME ignored)
with configuration file, and it does not contain a name	k3s-default	NAME	existing cluster names	existing cluster names (NAME ignored)
with configuration file and it contains a name	name from configuration file	NAME (name from configuration file ignored)	existing cluster names (name from configuration file ignored)	existing cluster names (NAME and name from configuration file ignored)

I can do this change if you agree. It is worth mentioning that it would make it more consistent with cluster create, which indeed overrides the name from the configuration file if it is specified on the command line.

Let me know.

[dprunier]

@iwilltry42, any update on the above ? Thanks.

[iwilltry42]

I'm really sorry @dprunier I just got to look at this.
You're absolutely right and table 3 is what I was aiming for 👍

[dprunier]

@iwilltry42, no worries, can't say I've been very quick either :) I've checked in the changes to implement the behavior of table 3.

The test suite seems to pass.

Let me know what you think.

[iwilltry42]

Yep, looks good!
Thanks a lot for this :)