-
Notifications
You must be signed in to change notification settings - Fork 72
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 support for variables in bundle config #359
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.
Interpolation will not be trivial as it is here when variables are a struct. Think about having a short hand for variable interpolation through var.foo
if it would otherwise be variables.foo.value
without further treatment.
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.
Don't have too much context but thanks for having me review!
Co-authored-by: Miles Yucht <miles@databricks.com>
Co-authored-by: Miles Yucht <miles@databricks.com>
Co-authored-by: Miles Yucht <miles@databricks.com>
cmd/bundle/schema.go
Outdated
@@ -14,6 +14,9 @@ var schemaCmd = &cobra.Command{ | |||
Short: "Generate JSON Schema for bundle configuration", | |||
|
|||
RunE: func(cmd *cobra.Command, args []string) error { | |||
// mark the --var flag as hidden since it's not relevant to this command | |||
cmd.Flags().MarkHidden("var") | |||
|
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 happens too late. As is, schema --help
will still display it. Has to happen in the init
function below.
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.
It does not work from inside the init
function. My hypothesis is that you cannot hide persistent flags
In this case, I would suggest let's just let the --var
flag be visible for the schema command
Here is what the help output looks like:
shreyas.goenka@THW32HFW6T bricks % bricks bundle schema --help
Generate JSON Schema for bundle configuration
Usage:
bricks bundle schema [flags]
Flags:
-h, --help help for schema
--only-docs only generate descriptions for the schema
--openapi string path to a databricks openapi spec
Global Flags:
-e, --environment string bundle environment to use (if applicable)
--log-file file file to write logs to (default stderr)
--log-format type log output format (text or json) (default text)
--log-level format log level (default disabled)
-o, --output type output type: text or json (default text)
-p, --profile string ~/.databrickscfg profile
--progress-format format format for progress logs (append, inplace, json) (default default)
--var strings set values for variables defined in bundle config. Example: --var="foo=bar"
Arguments for skipping this:
- We should other flags like --profile, --output and the --environment flag which are not relevant for the schema command as well
- It's nested under the Global Flags section which is less of a problem than if it was visible under the
Flags
sections schema
might be the only bundle command for which the--var
flag is not needed
Arguments against:
- Hiding persistent flags from parents in child commands is something that we might have to figure out in the future as well
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.
Nice work. Please TAL at remaining comments before merging.
if _, ok := r.Variables[name]; !ok { | ||
return fmt.Errorf("variable %s has not been defined", name) | ||
} | ||
err := r.Variables[name].Set(val) |
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.
Question: should we allow this if the configuration has already provided a value?
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 follows the semantics from how terraform handles variables. You can define a default value in the config, and override it from the command line flags / env vars / module input parameters
Do you have any specific concerns here?
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 difference with TF is that here the values can be hardcoded in the configuration and overridden per environment, for example. We need to have rule that says what happens if you try to set (through env or command argument) a variable with a value that has already been set.
Changes
This PR now allows you to define variables in the bundle config and set them in three ways
Tests
manually, unit, and black box tests