-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
issue edit: avoid race conditions when editing assignees, reviewers #9037
base: trunk
Are you sure you want to change the base?
issue edit: avoid race conditions when editing assignees, reviewers #9037
Conversation
Co-Authored-By: Mislav Marohnić <[email protected]>
…ace-conditions-when-editing-assignees,-reviewers-
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ... |
@ChrisSidebotham @wzshiming @jtracey93: have any of you had an opportunity to test this build locally as described to see whether it objectively addresses the organization scale problem reported? given the uniqueness of this problem, we appreciate your help ensure the root issue is addressed. if not, I hope we can suss out where the issue is and iterate on this PR before finally shipping. |
Continuing the work done by @mislav in #4876
Fixes #6235
Fixes #4844