Conversation
|
/gemini review |
Summary of ChangesHello @gurusai-voleti, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a persistent 'permadiff' problem within the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the CustomizeDiff logic for storage_bucket_acl to address permutation diffs, and adds logic to prevent deletion of more default ACLs. The new CustomizeDiff logic is a good improvement, but there are opportunities to make it more robust by adding type assertion checks and using type assertions instead of fmt.Sprint. The new logic in resourceStorageBucketAclDelete for skipping default ACLs contains a bug and should be fixed. I've also included a suggestion to improve the readability of the new isDefaultGcpAcl helper function.
| if res.Entity == fmt.Sprintf("project-editors-%s", project) && res.Role == "OWNER" { | ||
| log.Printf("[WARN]: Skipping %s-%s; not deleting owner ACL.", res.Role, res.Entity) | ||
| continue | ||
| } | ||
|
|
||
| if res.Entity == fmt.Sprintf("project-viewers-%s", project) && res.Role == "READER" { | ||
| log.Printf("[WARN]: Skipping %s-%s; not deleting owner ACL.", res.Role, res.Entity) | ||
| continue | ||
| } |
There was a problem hiding this comment.
The check res.Entity == fmt.Sprintf(...) is likely incorrect because it performs an exact match using the project ID, while the entity name for these default roles contains the project number. Using strings.HasPrefix is more robust. The two new checks can also be combined into one, and the log message not deleting owner ACL is inaccurate for the READER role and should be made more generic.
if (strings.HasPrefix(res.Entity, "project-editors-") && res.Role == "OWNER") ||
(strings.HasPrefix(res.Entity, "project-viewers-") && res.Role == "READER") {
log.Printf("[WARN]: Skipping %s-%s; not deleting default ACL.", res.Role, res.Entity)
continue
}| oldList, _ := oldData.([]interface{}) | ||
| newList, _ := newData.([]interface{}) |
There was a problem hiding this comment.
The type assertions to []interface{} for oldData and newData discard the ok boolean that indicates if the assertion was successful. If the assertion fails, oldList or newList will be nil. The current code handles this by checking len() == 0, but it silently treats invalid data as an empty list. It's safer and better practice to explicitly check the ok value and return an error if the type is not what's expected.
oldList, ok1 := oldData.([]interface{})
newList, ok2 := newData.([]interface{})
if (!ok1 && oldData != nil) || (!ok2 && newData != nil) {
return fmt.Errorf("role_entity was not a valid list")
}|
|
||
| newSet := make(map[string]struct{}) | ||
| for _, item := range newList { | ||
| newSet[fmt.Sprint(item)] = struct{}{} |
There was a problem hiding this comment.
The role_entity is a list of strings. Using fmt.Sprint(item) is less efficient and less type-safe than a direct type assertion to string. Please use a type assertion item.(string) instead. This will also make the code more robust by panicking if the type is unexpectedly different, which is desirable for revealing bugs during testing.
newSet[item.(string)] = struct{}{}| // Preserve order from oldList and handle removals, this will help avoid permadiff | ||
| // Iterate through the original list to maintain its order. | ||
| for _, item := range oldList { | ||
| key := fmt.Sprint(item) |
There was a problem hiding this comment.
Similar to a previous comment, it's better to use a type assertion item.(string) here instead of fmt.Sprint(item) for type safety and performance, as role_entity items are strings.
key := item.(string)| // Append any new additions | ||
| // Iterate through the new config to find items that weren't in the old list or newly added | ||
| for _, item := range newList { | ||
| key := fmt.Sprint(item) |
There was a problem hiding this comment.
Similar to previous comments, it's better to use a type assertion item.(string) here instead of fmt.Sprint(item) for type safety and performance.
key := item.(string)| func isDefaultGcpAcl(key string) bool { | ||
| if strings.HasPrefix(key, "OWNER:project-owners-") { | ||
| return true | ||
| } | ||
| if strings.HasPrefix(key, "OWNER:project-editors-") { | ||
| return true | ||
| } | ||
| if strings.HasPrefix(key, "READER:project-viewers-") { | ||
| return true | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
This function can be written more concisely by combining the conditions with a logical OR (||) operator in a single return statement. This improves readability and reduces the number of return points.
func isDefaultGcpAcl(key string) bool {
return strings.HasPrefix(key, "OWNER:project-owners-") ||
strings.HasPrefix(key, "OWNER:project-editors-") ||
strings.HasPrefix(key, "READER:project-viewers-")
}
There was a problem hiding this comment.
Code Review
This pull request refactors the CustomizeDiff logic for storage_bucket_acl to better handle default ACLs and prevent permadiffs. The new implementation preserves the order of existing ACLs and prevents the removal of default project-level ACLs for owners, editors, and viewers. The logic in resourceStorageBucketAclDelete is also updated to align with this change, and corresponding tests are added.
My feedback includes a suggestion to refactor the new isDefaultGcpAcl helper for better maintainability and to correct a misleading log message in resourceStorageBucketAclDelete for improved clarity during debugging.
| func isDefaultGcpAcl(key string) bool { | ||
| if strings.HasPrefix(key, "OWNER:project-owners-") { | ||
| return true | ||
| } | ||
| if strings.HasPrefix(key, "OWNER:project-editors-") { | ||
| return true | ||
| } | ||
| if strings.HasPrefix(key, "READER:project-viewers-") { | ||
| return true | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
The isDefaultGcpAcl function can be made more maintainable and readable by using a slice of prefixes and iterating over it. This approach makes it easier to add or remove default ACL prefixes in the future without adding more if statements.
| func isDefaultGcpAcl(key string) bool { | |
| if strings.HasPrefix(key, "OWNER:project-owners-") { | |
| return true | |
| } | |
| if strings.HasPrefix(key, "OWNER:project-editors-") { | |
| return true | |
| } | |
| if strings.HasPrefix(key, "READER:project-viewers-") { | |
| return true | |
| } | |
| return false | |
| } | |
| func isDefaultGcpAcl(key string) bool { | |
| defaultPrefixes := []string{ | |
| "OWNER:project-owners-", | |
| "OWNER:project-editors-", | |
| "READER:project-viewers-", | |
| } | |
| for _, p := range defaultPrefixes { | |
| if strings.HasPrefix(key, p) { | |
| return true | |
| } | |
| } | |
| return false | |
| } |
| if res.Entity == fmt.Sprintf("project-editors-%s", project) && res.Role == "OWNER" { | ||
| log.Printf("[WARN]: Skipping %s-%s; not deleting owner ACL.", res.Role, res.Entity) | ||
| continue | ||
| } | ||
|
|
||
| if res.Entity == fmt.Sprintf("project-viewers-%s", project) && res.Role == "READER" { | ||
| log.Printf("[WARN]: Skipping %s-%s; not deleting owner ACL.", res.Role, res.Entity) | ||
| continue | ||
| } |
There was a problem hiding this comment.
The log message "not deleting owner ACL" is misleading when applied to editor and viewer roles. Using a more generic term like "default ACL" would be more accurate and improve clarity when debugging.
| if res.Entity == fmt.Sprintf("project-editors-%s", project) && res.Role == "OWNER" { | |
| log.Printf("[WARN]: Skipping %s-%s; not deleting owner ACL.", res.Role, res.Entity) | |
| continue | |
| } | |
| if res.Entity == fmt.Sprintf("project-viewers-%s", project) && res.Role == "READER" { | |
| log.Printf("[WARN]: Skipping %s-%s; not deleting owner ACL.", res.Role, res.Entity) | |
| continue | |
| } | |
| if res.Entity == fmt.Sprintf("project-editors-%s", project) && res.Role == "OWNER" { | |
| log.Printf("[WARN]: Skipping %s-%s; not deleting default ACL.", res.Role, res.Entity) | |
| continue | |
| } | |
| if res.Entity == fmt.Sprintf("project-viewers-%s", project) && res.Role == "READER" { | |
| log.Printf("[WARN]: Skipping %s-%s; not deleting default ACL.", res.Role, res.Entity) | |
| continue | |
| } |
1 participant
Release Note Template for Downstream PRs (will be copied)
See Write release notes for guidance.