-
Notifications
You must be signed in to change notification settings - Fork 39
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: Updates SDK for database-user v20231115002 to v20231115008 #1014
Conversation
@@ -119,8 +119,7 @@ func Read(req handler.Request, prevModel *Model, currentModel *Model) (handler.P | |||
_, _ = logger.Debugf("databaseUser:%+v", databaseUser) | |||
var roles []RoleDefinition | |||
|
|||
for i := range databaseUser.Roles { | |||
r := databaseUser.Roles[i] | |||
for _, r := range databaseUser.GetRoles() { | |||
role := RoleDefinition{ | |||
CollectionName: r.CollectionName, | |||
DatabaseName: &r.DatabaseName, |
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.
can it also be used Get functions 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.
Seems a bit weird, since the RoleDefinition
expects a pointer. WDYT?
@@ -133,8 +132,7 @@ func Read(req handler.Request, prevModel *Model, currentModel *Model) (handler.P | |||
|
|||
var labels []LabelDefinition | |||
|
|||
for i := range databaseUser.Labels { | |||
l := databaseUser.Labels[i] | |||
for _, l := range databaseUser.GetLabels() { | |||
label := LabelDefinition{ | |||
Key: l.Key, |
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.
same, GetKey(), etc. can be used here?
@@ -293,7 +290,7 @@ func List(req handler.Request, prevModel *Model, currentModel *Model) (handler.P | |||
} | |||
|
|||
func setModel(currentModel *Model) (*admin.CloudDatabaseUser, error) { | |||
var roles []admin.DatabaseUserRole | |||
roles := []admin.DatabaseUserRole{} |
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.
knit: better performance but not so important here
roles := make([]admin.DatabaseUserRole, 0, len(currentModel.Roles))
Labels: labels, | ||
Scopes: scopes, | ||
Labels: &labels, | ||
Scopes: &scopes, |
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.
knit: to make it clear that this is never nil and can take the pointer, you can define:
scopes := &[]admin.UserScope{} and then:
Scopes: scopes,
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.
hmm. After changing the definitions above, it is not possible to use the &
in the definition.
But not sure what is best practice.
Please let me know your preference :D
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.
ok as it is. it's a best practice when you do & to try to make it clear in the code (as close as possible to &) that it will never fail
@@ -15,6 +15,13 @@ Resources: | |||
Principal: | |||
Service: resources.cloudformation.amazonaws.com | |||
Action: sts:AssumeRole | |||
Condition: |
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.
why has this file changed?
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.
Did a make build
.
cfn generate
changes this file.
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.
thanks, i guessed that, but this probably means that we didn't do it in previous PRs
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.
LGTM. Regarding the approach of a PR per resource, would doing a group of resources per PR be more efficient? Just thinking that with this approach we would have 40+ PRs in total
f34b99c
@AgustinBettati @EspenAlbert i see the point of efficiency of doing grouped PRs, but also it's normally better to do focused PRs. Bigger PRs are harder to do and review, and easier to miss some possible bugs |
Proposed changes
Updates SDK for database-user v20231115002 to v20231115008
See the Jira issue for the upgrade strategy.
Link to any related issue(s): CLOUDP-222755
Type of change:
expected)
Manual QA performed:
Required Checklist:
make fmt
and formatted my codeworks in Atlas
Further comments