This repository has been archived by the owner on Feb 15, 2024. It is now read-only.
Update the Installable Build job status for Jetpack #90
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
AliSoftware
reviewed
Sep 6, 2021
org/pr/installable-build.ts
Outdated
Comment on lines
139
to
144
| if (status.state == "pending" && HOLD_CONTEXTS.includes(status.context)) { | ||
| if (status.state == "pending" && HOLD_CONTEXTS.concat(HOLD_CONTEXTS_NO_COMMENT).includes(status.context)) { | ||
| await markStatusAsSuccess(status) | ||
| await createOrUpdateComment(status, `You can trigger an installable build for these changes by visiting CircleCI [here](${status.target_url}).`) | ||
| } else if (status.state == "success" && INSTALLABLE_BUILD_CONTEXTS.filter(s => status.context.match(s)).length > 0) { |
There was a problem hiding this comment.
If I'm not mistaken, this is the only place where both HOLD_CONTEXTS and HOLD_CONTEXTS_NO_COMMENTS constants are used.
- If that's intended, and the only place where they are used here they are concatenated, not sure what's the point of having them split in the first place?
- Instead I think what you intended to do is to NOT call
createOrUpdateCommentfor the case ofHOLD_CONTEXTS_NO_COMMENTS?
There was a problem hiding this comment.
Oh, I didn't notice my last push failed 🤦! I guess this is what you get when you try to write some code while you have to respond to too many pings 😂
There was a problem hiding this comment.
Yeah! That version was clearly doing nothing :-)
AliSoftware
reviewed
Sep 6, 2021
There was a problem hiding this comment.
Aside from stylistic comment, LGTM, but I'll let @jkmassel approve since he's the one knowing what the original intent was here.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
After #89 got merged, I've started noticing that the status of the
Installable Buildjob for the Jetpack application doesn't get updated frompending (orange)toready (green).The reason is that this commit removed it from the list of the handled states.
My understanding, from the commit title, is that the goal was to avoid that Peril commented on the PR with the prompt to run the installable build for Jetpack.
I think we still want the state of the job to be green, because, otherwise, the PR remains in a pending state and, while it's still possible to merge the changes because the job is not required, this behaviour can create confusion.
My take is that the bug that #89 fixed was actually hiding this problem.
This PR should fix this problem. There are probably better ways to do this, and the test cases are not complete and strong, but since @jkmassel mentioned that with the migration to Buildkite this function will be integrated in the CI and won't be required anymore in Peril, I opted for a quick, minimal and easy solution.