issue edit: avoid race conditions when editing assignees, reviewers #9037
Conversation
Co-Authored-By: Mislav Marohnić <[email protected]>
…ace-conditions-when-editing-assignees,-reviewers-
cliAutomation
added
the
external
|
@wingleung : Thank you for taking the time to contribute to the GitHub CLI and for your patience! 🙇 I'm excited to see how you picked up Mislav's work ❤️ |
There was a problem hiding this comment.
Comparing these changes to #4876, I wonder what issues arose to block completion of the PR then. 🤔
@williammartin : I'd like a 2nd opinion if you wouldn't mind. I see this largely follows what @mislav was originally doing and moves to use actual user IDs than pulling assignableUsers, but would like a second opinion.
@ChrisSidebotham: given your recent interest, do you have any large organizations you can do testing with for additional feedback? 🙇
| if len(options.Assignees.Add) > 0 { | ||
| wg.Go(func() error { | ||
| addLogins, err := meReplacer.ReplaceSlice(options.Assignees.Add) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| addIDs, err := options.Metadata.MembersToIDs(addLogins) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return addAssignees(httpClient, repo, id, addIDs) | ||
| }) | ||
| } | ||
|
|
||
| if len(options.Assignees.Remove) > 0 { | ||
| wg.Go(func() error { | ||
| removeLogins, err := meReplacer.ReplaceSlice(options.Assignees.Remove) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| removeIDs, err := options.Metadata.MembersToIDs(removeLogins) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return removeAssignees(httpClient, repo, id, removeIDs) | ||
| }) | ||
| } |
There was a problem hiding this comment.
This logic is dependent on ReplaceValue() logic removing editable content from list of content to be added, allowing it to run concurrently without having the same content being added and removed, correct?
There was a problem hiding this comment.
the ReplaceValue() is only for interaction mode, where we get a list of selected users which may contain deselected users from the default state. The selected list will be compared to the default list and reduced to have correct values in the Add and Remove fields before we do the queries.
the ReplaceValue() is not used when we run the cli with --add-assignee and --remove-assignee flags, we handle both flows in their own goroutine and let the graphql mutation handle the data change.
this could introduce an edge case where --add-assignee wingleung --remove-assignee wingleung is a race condition with 2 possible outcomes.
Alternative would be to combine multiple mutations in 1 query, which would ensure the mutations run in order ℹ️.
However, another edge case would then be --remove-assignee wingleung --add-assignee wingleung.
We don't know the order of which the flags were given, so if we always do the addAssigneesToAssignable first and then the removeAssigneesFromAssignable then the last flag in our edge case will not do it's job.
I'm overthinking it, the user of the cli is responsible of using correct flag values, --add-assignee wingleung --remove-assignee wingleung is prolly not a real life use case. And afterwards we just need to pass it on to graphql as fast as we can 😄
|
@andyfeller - I would be happy to test this with the Azure org which is where we noticed the orignal issue |
Co-authored-by: Andy Feller <[email protected]>
Co-authored-by: Andy Feller <[email protected]>
…ace-conditions-when-editing-assignees,-reviewers-
@ChrisSidebotham : many thanks! because of the nuanced nature of scale to test against, I want to make sure you, @wzshiming, @jtracey93 can weigh in on the development build solving the issue. Just a reminder, you can build a given gh pr checkout 9037
make
bin/gh ... |
Continuing the work done by @mislav in #4876
Fixes #6235
Fixes #4844