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

Tweak environment flags #226

Merged
merged 1 commit into from
Dec 21, 2021
Merged

Tweak environment flags #226

merged 1 commit into from
Dec 21, 2021

Conversation

janelletavares
Copy link
Contributor

@janelletavares janelletavares commented Dec 8, 2021

Description of change

The current format for a config flag (and metadata) can be a bit error prone, so switching that to a string array format.

Also, introducing default values for environment create flags.

If we like it, can consider rolling out to other commands.

Fixes

Type of change

  • New feature
  • Bug fix
  • Refactor
  • Documentation

How was this tested?

  • [X ] Unit Tests
  • Tested in staging

Demo

Before this pull-request

Flags:
  -c, --config string     environment configuration based on type and provider (e.g.: --config '{"aws_access_key_id":"my_access_key", "aws_secret_access_key":"my_secret_access_key"}')

After this pull-request

Flags:
  -c, --config strings    environment configuration based on type and provider (e.g.: --config aws_access_key_id=my_access_key --config aws_access_secret=my_access_secret)

Copy link
Member

@raulb raulb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@janelletavares changes on --config look great.

I've got some questions on the default value (one of them being that I miss where we make use of it). Do you mind opening a separate PR only making the change of --config so we can iterate on each one independently? Thank you 🙏

@@ -105,19 +105,25 @@ func (c *Create) setUserValues(e *meroxa.CreateEnvironmentInput) {
}
}

func stringSliceToMap(input []string) map[string]interface{} {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be extracted in some point as part of builder, once we change how config flags are used in other commands, but I think it's fine we leave it here for now.

@janelletavares
Copy link
Contributor Author

janelletavares commented Dec 15, 2021

@janelletavares changes on --config look great.

I've got some questions on the default value (one of them being that I miss where we make use of it). Do you mind opening a separate PR only making the change of --config so we can iterate on each one independently? Thank you pray

The ConfigSlice support is something I pulled out of my champagne day PR, and defaults was in there as well and when I started it felt relevant so I also copied it over. But, after testing, I removed using the defaults for the environment create command because it wasn't producing reasonable behavior with the prompt. However, having it as an option in general does seem nice, reduces some looking up of valid values and some typing and therefore opportunities for error, so i didn't delete all of it. i will open a separate PR for that. Thanks for looking!

Copy link
Member

@raulb raulb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 thank you!

@janelletavares janelletavares merged commit 615fca1 into master Dec 21, 2021
@janelletavares janelletavares deleted the array-flag branch December 21, 2021 01:06
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.

2 participants