feat: Refactor repository collaborators#3233
feat: Refactor repository collaborators#3233stevehipwell wants to merge 1 commit intointegrations:mainfrom
Conversation
stevehipwell
added
Type: Bug
|
👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labeled with |
stevehipwell
force-pushed
the
refactor-repo-collaborators
branch
2 times, most recently
from
55043eb to
4be7e25
Compare
deiga
left a comment
deiga
left a comment
There was a problem hiding this comment.
Partial review. Loving the direction here!
| if _, ok := seen[username]; ok { | ||
| return fmt.Errorf("duplicate username %s found in user collaborators", username) | ||
| } |
There was a problem hiding this comment.
question: Since "user" is TypeSet is it even possible to add duplicates? Oh, only if their hashes match
We could use the Set schema field to use only the username to calculate the hash, then this check wouldn't be necessary.
NOTE: Though, it doesn't error or complain, it just silently adds the first(?) of the duplicates
Set: func(v interface{}) int {
m := v.(map[string]interface{})
return schema.HashString(m["username"].(string))
},
There was a problem hiding this comment.
We can't use Set as it errors if the values aren't known at plan time. I did spend a bit of time testing this out and seeing if I could work around it, but this behaviour (and better validation) needs the framework.
| if d.HasChange("team") && d.NewValueKnown("team") { | ||
| v, diags := d.GetRawConfigAt(cty.GetAttrPath("team")) | ||
| if diags.HasError() { | ||
| return fmt.Errorf("error reading team config: %v", diags) | ||
| } | ||
|
|
||
| if !v.IsNull() && v.IsKnown() { | ||
| seen := make(map[string]any) | ||
| it := v.ElementIterator() | ||
| for it.Next() { | ||
| _, elem := it.Element() | ||
| val := elem.GetAttr("team_id") | ||
| if val.IsNull() || !val.IsKnown() { | ||
| continue | ||
| } | ||
|
|
||
| teamID := val.AsString() | ||
| if _, ok := seen[teamID]; ok { | ||
| return fmt.Errorf("duplicate team %s found in team collaborators", teamID) | ||
| } | ||
| seen[teamID] = nil | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
question: This seems way too complex for the benefit. Would it make sense to simplify by using the Set: field to change the Hash calculation?
There was a problem hiding this comment.
As above, this isn't possible with the plugin SDK.
Signed-off-by: Steve Hipwell <steve.hipwell@gmail.com>
stevehipwell
force-pushed
the
refactor-repo-collaborators
branch
from
4be7e25 to
42a9206
Compare
2 participants
Resolves #2874
Before the change?
github_repository_collaboratorserrored if the repo was renamedgithub_repository_collaboratorsprefered team ID and made extra calls if slug was usedgithub_repository_collaboratorsused unnecessary API calls to re-readAfter the change?
github_repository_collaboratorssupports the repository being renamedgithub_repository_collaboratorsuses the minimum number of API callsPull request checklist
Does this introduce a breaking change?
Please see our docs on breaking changes to help!