-
Notifications
You must be signed in to change notification settings - Fork 395
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
apps: Nest dev-config settings under 'dev' #1286
Conversation
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.
LGTM!
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.
good catch and fix on this -- I'm on board with the current solution as a fast fix
b879d3c
b879d3c
to
ae75fd9
Compare
ae75fd9
to
dbfb6ec
Compare
Pushed an additional commit to not nest deeper if it is already under |
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.
LGTM
Am waiting for what to do |
The
flagName
method that is used when binding flags to viper only supports nesting a setting three levels deep.doctl/commands/doit.go
Lines 287 to 292 in de1918a
This causes issues for some deeply nested commands. This is particularly problematic for the
doctl apps dev config set/unset
commands which contain--dev-config
flag. The resulting setting for it isconfig.set.dev-config
which conflicts with the top levelconfig
setting that should be a string.When Viper is init-ed, there is a race to register the setting. Sometimes, it results in the config file containing:
Others, it has the appropriate:
This breaks things in bad ways causing
doctl auth init
to fail unpredictably.This PR forces the
dev-config
setting to be nested more deeply, i.e.dev.config.set.dev-config
. I don't entirely love this solution, but a more general one would be a breaking change to the configuration file format For example, the settings for DOKS are likecluster.create.region
rather thankubernetes.cluster.create.region
. I think we should prioritize getting this specific problem fixed over a broader solution.Fixes: #1281, #1280