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

Fix cml-pr reference handling for pull requests #624

Merged
merged 4 commits into from
Jul 4, 2021

Conversation

0x2b3bfa0
Copy link
Member

@0x2b3bfa0 0x2b3bfa0 commented Jun 29, 2021

Workflows triggered with the pull_request GitHub Actions event are going to have github.ref set to one of the pull references that GitHub creates for pull requests. We haven't noticed until now because all the tests were made using the push event.

We only consider heads and tags references and remove the first path components with a regular expression, and it works pretty well for these two kinds of references, but fails for the pull ones:

return branch.replace(/refs\/(head|tag)s\//, '');

The most elegant solution I can think of would using a short-circuit evaluation fallback to GITHUB_HEAD_REF || GITHUB_REF so it doesn't have to deal with pull references at all.

@0x2b3bfa0 0x2b3bfa0 added bug Something isn't working cml-pr Subcommand labels Jun 29, 2021
@0x2b3bfa0 0x2b3bfa0 self-assigned this Jun 29, 2021
@0x2b3bfa0 0x2b3bfa0 temporarily deployed to internal June 29, 2021 19:53 Inactive
@DavidGOrtega DavidGOrtega added the p0-critical Max priority (ASAP) label Jun 30, 2021
@0x2b3bfa0 0x2b3bfa0 temporarily deployed to internal June 30, 2021 15:28 Inactive
@0x2b3bfa0 0x2b3bfa0 temporarily deployed to internal June 30, 2021 15:29 Inactive
@0x2b3bfa0 0x2b3bfa0 marked this pull request as ready for review June 30, 2021 15:29
src/cml.js Outdated Show resolved Hide resolved
@casperdcl
Copy link
Contributor

#630 wasn't needed (same as #623)

@0x2b3bfa0 0x2b3bfa0 temporarily deployed to internal June 30, 2021 17:40 Inactive
@0x2b3bfa0 0x2b3bfa0 temporarily deployed to internal June 30, 2021 18:04 Inactive
@0x2b3bfa0 0x2b3bfa0 temporarily deployed to internal June 30, 2021 18:37 Inactive
@0x2b3bfa0 0x2b3bfa0 force-pushed the fix-pull-request-references branch from 408a306 to 26b90b5 Compare June 30, 2021 20:06
@0x2b3bfa0 0x2b3bfa0 temporarily deployed to internal June 30, 2021 20:06 Inactive
@0x2b3bfa0 0x2b3bfa0 requested a review from casperdcl June 30, 2021 22:31
@0x2b3bfa0 0x2b3bfa0 force-pushed the fix-pull-request-references branch from 26b90b5 to 961aa4c Compare June 30, 2021 23:07
@0x2b3bfa0 0x2b3bfa0 temporarily deployed to internal June 30, 2021 23:07 Inactive
@0x2b3bfa0 0x2b3bfa0 temporarily deployed to internal June 30, 2021 23:08 Inactive
@0x2b3bfa0 0x2b3bfa0 force-pushed the fix-pull-request-references branch from 3384855 to 90cd1c8 Compare June 30, 2021 23:24
@0x2b3bfa0 0x2b3bfa0 temporarily deployed to internal June 30, 2021 23:24 Inactive
src/cml.js Outdated Show resolved Hide resolved
@0x2b3bfa0 0x2b3bfa0 temporarily deployed to internal July 1, 2021 11:01 Inactive
Copy link
Contributor

@DavidGOrtega DavidGOrtega left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@DavidGOrtega DavidGOrtega merged commit 2dfbf87 into master Jul 4, 2021
@DavidGOrtega DavidGOrtega deleted the fix-pull-request-references branch July 4, 2021 21:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working cml-pr Subcommand p0-critical Max priority (ASAP)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cml-pr fails with Validation Failed: {"resource":"PullRequest","field":"base","code":"invalid"}
3 participants