allow for configurable update stream #101
Conversation
This commit has a few components: 1. Some YAML utils: one for inserting values into generic YAML using dot-notation keys, and one for validating that YAML can be deserialized as a given object type. 2. Introduces a new ~/.klotho/options.yaml for CLI options. These are strictly typed within the code, but we're more lenient in terms of configs we allow setting. In particular, we allow setting configs that we don't yet know about; this can be helpful in cases where you want to set a config that's available in a subsequent version, before you upgrade to that version. We do warn on this though: in manual testing, I had done "upgrade.stream" instead of "update.stream" and was confused why this didn't work, so I figured I'd help out the next person who runs into that. 3. KlothoMain introduces a new flag, `--set-option name=value`. You can use it on its own, or inline it with `--update`. I'm also getting rid of updater.go's DefaultStream var, because that really needs to be specified only by the KlothoMain instantiation. This resolves #78
pkg/cli/cli_options_test.go
Outdated
| warns []string | ||
| } | ||
|
|
||
| func (ml *mockedLogger) Warn(msg string, fields ...zap.Field) { |
There was a problem hiding this comment.
I use a zap observer to test log output if you think its less ugly. I think its personally a bit cleaner than this implementation, but up to you
This was just me trying to remind myself how errors.Is works.
| Version string | ||
| VersionQualifier string | ||
| PluginSetup func(*PluginSetBuilder) error | ||
| DefaultUpdateStream string |
There was a problem hiding this comment.
nit: Is this the default if they have set it in their config?
There was a problem hiding this comment.
I think so? It's the default, which they can then override in their options. (It's actually not overridable on a one-invocation-only basis, only via persisted options.)
(As an aside: I'm trying to use "options" for the CLI-wide stuff [which is currently only the update stream], to differentiate it from the klotho project configs -c.)
| topNode = tree.Content[0] // the tree's root is a DocumentNode; we assume one document | ||
| } | ||
| setOptionAtNode := topNode | ||
| for _, segment := range segments[:len(segments)-1] { |
There was a problem hiding this comment.
for 2a) are we finding the value at the non leaf path? ie up to bar in this scenario? I assume thats the intention, just making sure
There was a problem hiding this comment.
Correct. We want the node right before the last segment, and then in 2b we set its value at that last segment.
So in fizz.buzz.foo = 123, the 2a loop gets you the fizz.buzz node, and then 2b sets *buzzNode["foo"] => 123
| } else if warns := yaml_util.CheckValid[Options](optionsYaml, yaml_util.Strict); warns != nil { | ||
| logger.Warn(`Existing options contain extra parameters:`) | ||
| for _, e := range yaml_util.YamlErrors(warns) { | ||
| logger.Warnf(`▸ %s`, e) |
There was a problem hiding this comment.
why only warn instead of raising actual errors? Wouldnt this signal an incorrect usage of the set-options flag?
There was a problem hiding this comment.
It probably signals incorrect usage, yes. But the incorrectness is just that it won't have any effect, not that it will have the wrong effect. So I figure a nice yellow warning is sufficient for that.
From previous experience, I've found that occasionally you want the ability to set a config that doesn't yet exist, which will take effect on the next update. For example, the next version of the tool may have an automatic migration that you want to modify. In cases like this, being able to specify a not-yet-existent config can be very useful. This approach also means that if you temporarily downgrade your version (once we have version pinning), you won't break your options file if you had set an option in 0.5.21 that doesn't exist in the 0.5.20 you've downgraded to.
Basically, I've found that being strict about "no unknown options" can get people into sticky situations, and that a nice clear warning is sufficient for catching typos.
|
ghost
merged commit 
This commit has a few components:
--set-option name=value. You can use it on its own, or inline it with--update.I'm also getting rid of updater.go's DefaultStream var, because that really needs to be specified only by the KlothoMain instantiation.
This resolves #78
Standard checks
mockableLogto cli_options.go, so that I could test the warning messages. I consider these to be important from a usability perspective, but I'm open to opinions here — especially on how else to do it (would a func be better? or I could actually tap into the zap logger — it's possible to do that, though it's a bit messy)--helptext