-
Notifications
You must be signed in to change notification settings - Fork 206
chore: Add autogen type override support for sets and lists #3775
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
chore: Add autogen type override support for sets and lists #3775
Conversation
|
||
func configBasic(orgID, name, description string, roles []string, secretExpiresAfterHours int) string { | ||
rolesStr := fmt.Sprintf(`%q`, strings.Join(roles, `", "`)) | ||
rolesStr := `"` + strings.Join(roles, `", "`) + `"` |
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.
Fixing non-matching escapes
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.
was this also incorrect in non-api resource test, to fix it there too?
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.
No handwritten resource for this one yet. I did check that we are using this in other tests.
cc755b3
to
752cdb6
Compare
func applyTypeOverride(override *config.Override, attr *Attribute) { | ||
switch *override.Type { | ||
case config.Set: | ||
if attr.List != nil { |
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.
should we throw an error if trying to override to set something that is not a list? same for list
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.
Currently we ignore it and print a warning, similar to applyTimeoutConfig() below.
+1 on moving to errors - ideally in another PR since we need a refactor.
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.
good to have it in another PR
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.
+1 for errors in this case, warnings may be easy to miss in automation
overrides: | ||
list_string: | ||
type: set | ||
description: "List overridden to set" |
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.
what's description used for?
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.
Just indicating that we are overriding the type. Could have gone for a comment 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.
i'd probably go for a comment if we're not using the field
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.
Done 941ed34
- Test Singleton Resource | ||
"/api/atlas/v2/groups/{groupId}/testResourceWithCollections": | ||
delete: | ||
description: DELETE API description |
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.
similarly as in another comment, what is description used for 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.
We set them for all operations, kept for consistency.
Description
Allow overriding an attribute's type in the autogen config.
Currently supporting
set
tolist
andlist
toset
overrides.Overriding the
roles
attribute in theorg_service_account_api
resource fromlist
toset
. This is necessary since the API spec is missing theuniqueItems: true
configuration (see CLOUDP-348768).Enabling the
TestAccOrgServiceAccountAPI_rolesOrdering
acceptance test.Link to any related issue(s): CLOUDP-349935
Type of change:
Required Checklist:
Further comments