Skip to content
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

Project Management Automation: Tolerate duplicate milestone #20011

Merged
merged 1 commit into from Feb 3, 2020

Conversation

aduth
Copy link
Member

@aduth aduth commented Feb 3, 2020

Related (possibly blocks): #20009

This pull request seeks to resolve an issue where the milestone assignment automation will fail to assign the milestone to a pull request when attempting to create the milestone would result in a duplicate milestone error. Prior to #17080, a duplicate milestone was considered tolerable. If the milestone already exists, it simply proceeds to assign the milestone. However, since this milestone creation would throw an error in the new JavaScript implementation, there was no chance for the task to recover to proceed to assigning the milestone.

With these changes, a thrown error is tolerated within the task, as long as it represents an error from a duplicate milestone.

Note: A possible future refactoring to this task could try to fetch the milestone first, and only if determined to not exist, proceed to create the milestone.

Testing Instructions:

This can only be tested once merged, confirming that a milestone is assigned.

@aduth aduth added [Type] Bug An existing feature does not function as intended [Type] Project Management Meta-issues related to project management of Gutenberg labels Feb 3, 2020
@aduth aduth requested a review from noisysocks February 3, 2020 19:00
@aduth aduth merged commit 8fd1bce into master Feb 3, 2020
@aduth aduth deleted the fix/add-milestone-tolerate-duplicate branch February 3, 2020 19:48
@aduth
Copy link
Member Author

aduth commented Feb 3, 2020

The task is still failing, despite this having intended to fix it.

https://github.com/WordPress/gutenberg/runs/423919172?check_suite_focus=true

I think the issue is that the code property is not actually part of the error instance, but rather passed along from the message reported in the response body of the API request.

https://developer.github.com/v3/#client-errors

It's also noted to be deprecated:

Deprecation: [@octokit/request-error] error.code is deprecated, use error.status.

(Not sure exactly what they mean by deprecated here, if it doesn't work at all)

I should also plan to follow-up at some point with additional unit tests here, to try to cover these scenarios rather than playing "Guess and Check" with the merges 😅 (though the existing test setup requires some manual mocking which would be equally as easy to mess up)

@epiqueras
Copy link
Contributor

error.code and error.status are the HTTP error code.

We need to read the response body.

@aduth
Copy link
Member Author

aduth commented Feb 3, 2020

Based on some further digging, we should be able to read an errors property from the error object.

See also:

I expect it would be the same as if we were to parse the response ourselves.

Pull request at: #20014

@ellatrix ellatrix added this to the Gutenberg 7.5 milestone Feb 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug An existing feature does not function as intended [Type] Project Management Meta-issues related to project management of Gutenberg
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants