-
Notifications
You must be signed in to change notification settings - Fork 171
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
feat: add --config
flag, an alternative to ZARF_CONFIG
env var
#2328
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for zarf-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Hey @waveywaves sorry for the delay but I would like to get this merged in at some point, I think it's pretty close. Could you merge main into this branch? There are a lot of conflicts but they're all from the new docs site so merging in main and running make docs-and-schema should fix it. Also could you add the following line to the appropriate block in src/cmd/internal.go
|
5a57bd4
to
1e5f744
Compare
if !argsConfigPathIsPresent() { | ||
if cfgFile != "" { | ||
// Use config file from the flag. | ||
v.SetConfigFile(cfgFile) | ||
} else { | ||
// Search config paths in the current directory and $HOME/.zarf. | ||
v.AddConfigPath(".") | ||
v.AddConfigPath("$HOME/.zarf") | ||
v.SetConfigName("zarf-config") | ||
} | ||
} |
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.
A lot of this block is duplicated directly under this
func SetViperConfigFilePath(vcfgFilePath string) { | ||
if vcfgFilePath != "" { | ||
v.SetConfigFile(config.CommonOptions.ConfigPath) | ||
vConfigError = v.ReadInConfig() | ||
} | ||
} |
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.
IMO it would be more clear to inline this function
@@ -570,7 +571,7 @@ $ zarf tools update-creds \ | |||
--artifact-push-username={USERNAME} --artifact-push-token={PASSWORD} | |||
|
|||
# NOTE: Any credentials omitted from flags without a service key specified will be autogenerated - URLs will only change if specified. | |||
# Config options can also be set with the 'init' section of a Zarf config file. | |||
# ConfigPath options can also be set with the 'init' section of a Zarf config file. |
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.
# ConfigPath options can also be set with the 'init' section of a Zarf config file. |
Let's just delete this comment, it seems that the config it's referring to is not the init file.
|
||
e2e.CleanFiles(path) | ||
|
||
// Test the config file flag | ||
os.Unsetenv("ZARF_CONFIG") |
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.
This was originally necessary but can now be deleted. While it won't break anything, each test should not expect environment variables to have been set by other tests.
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.
Thank you for getting back to this, it is a great feature to have
@@ -38,15 +44,17 @@ func TestConfigFile(t *testing.T) { | |||
e2e.CleanFiles(path) | |||
} | |||
|
|||
func configFileTests(t *testing.T, dir, path string) { | |||
func configFileTests(t *testing.T, dir, path string, configPath string) { |
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.
A bit of a nit, but I think it would be simpler if the function signature was
func configFileTests(t *testing.T, dir, path string, extraArgs ...string)
Then it could be called like below
configFileTests(t, dir, path, "--config-path", configPath)
and the args could simply be appended like this
args := append([]string{"package", "create", dir, "--confirm"}, extraArgs...)
--config
flag, an alternative to ZARF_CONFIG
env var--config
flag, an alternative to ZARF_CONFIG
env var
Description
add
--config
flag, an alternative toZARF_CONFIG
env varRelated Issue
Fixes #2178
Type of change
Checklist before merging