Skip to content
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

Merged
merged 30 commits into from
May 15, 2023
Merged

Add support for variables in bundle config #359

merged 30 commits into from
May 15, 2023

Conversation

shreyas-goenka
Copy link
Contributor

@shreyas-goenka shreyas-goenka commented Apr 25, 2023

Changes

This PR now allows you to define variables in the bundle config and set them in three ways

  1. command line args
  2. process environment variable
  3. in the bundle config itself

Tests

manually, unit, and black box tests

Copy link
Contributor

@pietern pietern left a 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.

bundle/config/mutator/load_variables_from_environment.go Outdated Show resolved Hide resolved
bundle/config/root.go Outdated Show resolved Hide resolved
bundle/config/mutator/load_variables_from_environment.go Outdated Show resolved Hide resolved
@shreyas-goenka shreyas-goenka requested review from mgyucht and nfx May 2, 2023 09:41
Copy link
Contributor

@mgyucht mgyucht left a 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!

libs/flags/variable_flag.go Outdated Show resolved Hide resolved
bundle/config/variables.go Outdated Show resolved Hide resolved
bundle/config/variables.go Outdated Show resolved Hide resolved
bundle/config/mutator/set_variables.go Outdated Show resolved Hide resolved
cmd/bundle/deploy.go Outdated Show resolved Hide resolved
bundle/phases/initialize.go Outdated Show resolved Hide resolved
shreyas-goenka and others added 3 commits May 2, 2023 13:19
Co-authored-by: Miles Yucht <miles@databricks.com>
Co-authored-by: Miles Yucht <miles@databricks.com>
Co-authored-by: Miles Yucht <miles@databricks.com>
nfx
nfx previously requested changes May 2, 2023
libs/flags/variable_flag.go Outdated Show resolved Hide resolved
cmd/bundle/deploy.go Outdated Show resolved Hide resolved
cmd/bundle/deploy.go Outdated Show resolved Hide resolved
cmd/bundle/deploy.go Outdated Show resolved Hide resolved
libs/flags/variable_flag.go Outdated Show resolved Hide resolved
cmd/bundle/deploy.go Show resolved Hide resolved
bundle/config/variables.go Outdated Show resolved Hide resolved
bundle/config/variables.go Outdated Show resolved Hide resolved
@shreyas-goenka shreyas-goenka requested a review from nfx May 4, 2023 14:58
bundle/config/interpolation/interpolation.go Outdated Show resolved Hide resolved
bundle/config/interpolation/lookup.go Outdated Show resolved Hide resolved
bundle/config/mutator/set_variables.go Show resolved Hide resolved
bundle/config/mutator/set_variables.go Outdated Show resolved Hide resolved
bundle/config/mutator/set_variables.go Outdated Show resolved Hide resolved
bundle/config/variables/variables.go Outdated Show resolved Hide resolved
bundle/config/mutator/set_variables.go Show resolved Hide resolved
cmd/bundle/run.go Outdated Show resolved Hide resolved
@@ -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")

Copy link
Contributor

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.

Copy link
Contributor Author

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:

  1. We should other flags like --profile, --output and the --environment flag which are not relevant for the schema command as well
  2. It's nested under the Global Flags section which is less of a problem than if it was visible under the Flags sections
  3. schema might be the only bundle command for which the --var flag is not needed

Arguments against:

  1. Hiding persistent flags from parents in child commands is something that we might have to figure out in the future as well

cmd/bundle/variables.go Outdated Show resolved Hide resolved
cmd/bundle/variables.go Outdated Show resolved Hide resolved
Copy link
Contributor

@pietern pietern left a 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.

bundle/config/mutator/set_variables_test.go Outdated Show resolved Hide resolved
if _, ok := r.Variables[name]; !ok {
return fmt.Errorf("variable %s has not been defined", name)
}
err := r.Variables[name].Set(val)
Copy link
Contributor

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?

Copy link
Contributor Author

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?

Copy link
Contributor

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.

bundle/config/variables/variables.go Outdated Show resolved Hide resolved
cmd/bundle/root.go Outdated Show resolved Hide resolved
@shreyas-goenka shreyas-goenka dismissed nfx’s stale review May 15, 2023 09:33

Addressed all comments

@shreyas-goenka shreyas-goenka merged commit c5e940f into main May 15, 2023
4 checks passed
@shreyas-goenka shreyas-goenka deleted the variables branch May 15, 2023 09:34
@shreyas-goenka shreyas-goenka mentioned this pull request May 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants