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

Feat: Adding JSON Format to configuration variables #196

Merged
merged 9 commits into from
Jan 3, 2022

Conversation

avnerenv0
Copy link
Contributor

@avnerenv0 avnerenv0 commented Dec 30, 2021

Issue & Steps to Reproduce / Feature Request

Add support for JSON Format configuration variables

Solution

  • Added JSON support - trying to keep it DRY between HCL and JSON where possible.
  • Added TEST_PATTERN option when running provider tests, since those take like 10 minutes

@avnerenv0
Copy link
Contributor Author

Copy link
Contributor

@RLRabinowitz RLRabinowitz left a comment

Choose a reason for hiding this comment

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

Looks good. Just one question, making sure there's no lingering bug here

if variable.Schema.Format == client.HCL {
d.Set("format", string(client.HCL))
if variable.Schema.Format != "" {
d.Set("format", variable.Schema.Format)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this whole logic be duplicated to environment resource's configuration variables as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

@RLRabinowitz looks like it's already there...

configurationSchema := client.ConfigurationVariableSchema{
Type: "string",
Format: client.Format(variable["schema_format"].(string)),
Enum: nil,
}

Copy link
Contributor

@liranfarage89 liranfarage89 left a comment

Choose a reason for hiding this comment

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

Well done! 🚀

Comment on lines +9 to +15
func ValidateConfigurationPropertySchema(val interface{}, key string) (warns []string, errs []error) {
value := val.(string)
if value != string(client.HCL) && value != string(client.Text) && value != string(client.JSON) {
errs = append(errs, fmt.Errorf("%q can be either \"HCL\", \"JSON\" or empty, got: %q", key, value))
}
return
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice

Comment on lines 101 to 107
var mockWithFormat = ConfigurationVariable{}
copier.Copy(&mockWithFormat, &mockConfigurationVariable)
mockWithFormat.Schema.Format = schemaFormat
requestWithFormat := GetExpectedRequest(mockWithFormat)

DoCreateRequest(mockWithFormat, requestWithFormat)

Copy link
Contributor

Choose a reason for hiding this comment

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

was not easy to follow here, maybe take requestWithFormat inside DoCreateRequest because they both use mockWithFormat?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed in 67d139d

LMK WDYT

@avnerenv0 avnerenv0 merged commit 1608c1b into main Jan 3, 2022
@avnerenv0 avnerenv0 deleted the feat-add-json-config-variable branch January 3, 2022 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants