Handle "Assignee has already been taken"

[ajaygm]

Handle the following response...
https://docs.github.com/en/free-pro-team@latest/rest/reference/issues#add-assignees-to-an-issue
HTTP Status Code: 422

{
	"message":"Validation Failed",
	"errors":["Could not add assignees: Validation failed: Assignee has already been taken"],
	"documentation_url":"https://developer.github.com/v3/issues/assignees/#add-assignees-to-an-issue"
}

Fixes #16145

[k8s-ci-robot]

Hi @ajaygm. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[matthyx]

/ok-to-test

[matthyx]

elegant solution, thanks!
/lgtm
/assign @alvaroaleman

[ajaygm]

@matthyx @alvaroaleman I was thinking of putting this functionality in the original "request" method and change the signature of the calling functions. With that I'll be able to get rid of this method as it is mostly doing whatever was done in the "request" method.
I wanted to create an independent PR to do these changes as they are not related to this bug fix.
Please let me know your thoughts

[alvaroaleman]

I don't think its a good idea to add this method or to have methods that have multiple error type returns that are mutually exclusive in general. The correct way for something like this would be to return the githubError as the last return and then use errors.As in the caller

[ajaygm]

Found out after a bit more digging in types.go that there was no need to extend githubError or to make any changes in the request function. There's already a datatype AlternativeClientError which has the fields corresponding to the response we get from Github. That data type is returned by request function, so just need to use errors.As as suggested above.

That raises an independent question - is there a need to have the data type githubError at all?

[alvaroaleman]

I don't think its a good idea to add this method or to have methods that have multiple error type returns that are mutually exclusive in general. The correct way for something like this would be to return the githubError as the last return and then use errors.As in the caller

[alvaroaleman]

I don't think we should be just ignoring that error, we should instead find out how it was possible that we tried to assign the same user twice

[ajaygm]

I agree with you @alvaroaleman
In fact I had asked for directions on places where I should try to look for the root cause of this issue - comment 1, comment 2, comment 3.
Please note that I'm not familiar with the codebase / workflow to make a judgement on places that could be triggering this type of behavior.

I was thinking that there are two approaches when looking at this type of issues...

1. This is a cloud based solution hence there are chances of calls being sent to your webhook multiple times. Your code should be resilient to such events and work correctly.
2. You should find out the root cause of why there were 2 calls in the first place.

As I didn't have much knowledge on how to deal with point 2 above, I took this approach to solve point 1 above. I have also left a debug message when we ignore the error incase we wish to still keep looking for the root cause. Is debug level message acceptable?

Please let me know your thoughts.

[alvaroaleman]

I was thinking that there are two approaches when looking at this type of issues...

1. This is a cloud based solution hence there are chances of calls being sent to your webhook multiple times. Your code should be resilient to such events and work correctly.
2. You should find out the root cause of why there were 2 calls in the first place.

Regarding 1), that is not expected to happen and if it would happen would cause a lot of other issues as well, like tests always get started twice.

Regarding 2), the place to look would be in the lgtm plugin somewhere:

test-infra/prow/plugins/lgtm/lgtm.go

Lines 149 to 177 in 91b8575

	func handleGenericCommentEvent(pc plugins.Agent, e github.GenericCommentEvent) error {
	cp, err := pc.CommentPruner()
	if err != nil {
	return err
	}
	return handleGenericComment(pc.GitHubClient, pc.PluginConfig, pc.OwnersClient, pc.Logger, cp, e)
	}
	func handlePullRequestEvent(pc plugins.Agent, pre github.PullRequestEvent) error {
	return handlePullRequest(
	pc.Logger,
	pc.GitHubClient,
	pc.PluginConfig,
	&pre,
	)
	}
	func handlePullRequestReviewEvent(pc plugins.Agent, e github.ReviewEvent) error {
	// If ReviewActsAsLgtm is disabled, ignore review event.
	opts := pc.PluginConfig.LgtmFor(e.Repo.Owner.Login, e.Repo.Name)
	if !opts.ReviewActsAsLgtm {
	return nil
	}
	cp, err := pc.CommentPruner()
	if err != nil {
	return err
	}
	return handlePullRequestReview(pc.GitHubClient, pc.PluginConfig, pc.OwnersClient, pc.Logger, cp, e)
	}

I am not sure if the underlying issue qualifies as good first issue

[ajaygm]

@alvaroaleman Are we sure about point 1 above not being possible? To rule that out, I've posted a question in the issue.

[alvaroaleman]

I meant it as in Github does give this guarantee and Prow would largely stop working properly if that wasn't the case. That being said its possible to configure the same webhook multiple times (for example on the github organization and repository) in which case the event will be sent once per webhook configuration.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ajaygm, petr-muller
To complete the pull request process, please assign alvaroaleman after the PR has been reviewed.
You can assign the PR to them by writing /assign @alvaroaleman in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• prow/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[spiffxp]

/test pull-test-infra-integration
#20498 (comment)

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle rotten

[fejta-bot]

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-contributor-experience at kubernetes/community.
/close

[k8s-ci-robot]

@fejta-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-contributor-experience at kubernetes/community.
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.