-
Notifications
You must be signed in to change notification settings - Fork 351
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
feat: [CYCLOUD-1447] Auto-detect PR numbers #1009
Conversation
Will you add a detailed description of what this PR is intended to achieve and how you will test it? You should read the CONTRIBUTING.md and apply the guidance in it. Note that this repo is based on npm not Yarn, so you shouldn't have a |
Yes, I will be sure to do that once this I have this in a state that's ready for review. |
Thanks very much for the description! Probably a shorter version of that should go into the README at some stage. You need to either enable HUSKY hooks or run npm ci
npm run format
npm run build to make sure that the code that you've written is actually the code which gets executed. I'll hold back on making any more comments unless you ask for review, since this is still a Draft PR.
|
@MikeMcC399 no problem! I was still working out some of the details when I had been pulled to something else; apologies for the delay there. Once we're certain this is working as we intend, the last part of this task is to update the docs with the changes, so I will be sure to update relevant docs before merging this one 👍 Appreciate the feedback here! |
…rmando/fix/CYCLOUD-1447
…rmando/fix/CYCLOUD-1447
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
|
Co-authored-by: Mike McCready <66998419+MikeMcC399@users.noreply.github.com>
The Cypress Cloud template for GitHub actions should also be updated in conjunction with this PR. Will you handle that process? If not, I would open a separate issue for that: Open https://cloud.cypress.io/ |
@MikeMcC399 good call, I'll work on getting those updated cloud-side. |
index.js
Outdated
// should have format refs/pull/<pr_number>/merge when triggered by pull_request workflow | ||
prNumber = parseInt(GITHUB_REF.split('/')[2]) | ||
} else { | ||
const resp = await client.request( |
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.
Might also be worth adding a try/catch around this request in the event of a partial outage or something. I don't think we'd want to halt a recording by throwing in that case.
README.md
Outdated
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
``` | ||
|
||
#### Branch with PR |
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.
Rather than focusing these sections around branches/PRs, it might make more sense to specify the behavior based on the various workflow event types.
For example, if the action executes on a push
event, we'll still need to make the request, even if there is a PR open, as the GITHUB_HEAD_REF env will not be set for that event.
We could do something like:
<example yaml here>
The behavior of the action varies based on the triggering event type.
#### Triggering event: `pull_request`/`pull_request_target`
...
#### Triggering event: `push`
...
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.
Good call! I'll update this 👍 That's much clearer.
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.
Left a few last minute comments. My testing has shown this all to be working great!
|
Co-authored-by: Mike McCready <66998419+MikeMcC399@users.noreply.github.com>
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.
Ran through my tests on the latest commit and all works as expected!
@MikeMcC399 we've had one approval so far - just wanted to get the thumbs up from you as well before we merge. Let me know if this is good with you! 😃 |
Co-authored-by: Mike McCready <66998419+MikeMcC399@users.noreply.github.com>
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.
Just one small documentation suggestion, otherwise LGTM! 👍🏻
Awesome, thanks! I committed the change - will merge this on green 👍 |
🎉 This PR is included in version 6.4.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
The Cypress GitHub action does not currently expose PR information in a way that is immediately-usable by the app (without further processing).
This PR updates the GitHub action so that it detects the PR number and passes it along in a way that can be consumed by the app. Currently we are targeting the existing environment variables,
CYPRESS_PULL_REQUEST_ID
&CYPRESS_PULL_REQUEST_URL
since both of these already exist and are currently in-use by both the app and cloud.NOTE: If the user has already set these variables, we will not overwrite them.
PR Number detection logic
listPullRequestsAssociatedWithCommit
APITesting
With PR
echo
-ed values match that of the PR from which the action was triggeredWith no direct PR, but with related PRs
echo
-ed values match that of the PR from the base branchWith no PRs at all
echo
-ed values are empty