Skip to content

feat: [CYCLOUD-1447] Auto-detect PR numbers#1009

Merged
ax-vasquez merged 44 commits into
masterfrom
armando/fix/CYCLOUD-1447
Sep 1, 2023
Merged

feat: [CYCLOUD-1447] Auto-detect PR numbers#1009
ax-vasquez merged 44 commits into
masterfrom
armando/fix/CYCLOUD-1447

Conversation

@ax-vasquez

@ax-vasquez ax-vasquez commented Aug 28, 2023

Copy link
Copy Markdown
Contributor

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

  1. When the GitHub event provides us with the PR info, we simply pass it along
  2. If the event doesn't have PR info, we instead grab the first PR related to the commit using GitHub's listPullRequestsAssociatedWithCommit API

Testing

For each of these test scenarios, start with the following steps:

  1. Set up a project with the Cypress GitHub action
  2. Customize your action so that it echos the CYPRESS_PULL_REQUEST_ID & CYPRESS_PULL_REQUEST_URL values

With PR

  1. Open a PR > push a commit and trigger the action
  2. Check the action output and ensure the echo-ed values match that of the PR from which the action was triggered

With no direct PR, but with related PRs

  1. Create a new branch from a branch that has a PR already
  2. Push the branch up to GitHub, but do not create a PR for the new branch
  3. Push the branch, but don't create a new commit (this new commit won't exist in the base PR if you do) > this should trigger a run
  4. Check the action output and ensure the echo-ed values match that of the PR from the base branch

With no PRs at all

  1. Close all open PRs
  2. Create a new branch
  3. Push a commit to trigger a new run
  4. Check the action output and ensure the echo-ed values are empty

@CLAassistant

CLAassistant commented Aug 28, 2023

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

Comment thread index.js Outdated
@MikeMcC399

Copy link
Copy Markdown
Collaborator

@ax-vasquez

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 yarn.lock in your PR. The repo uses package-lock.json.

@ax-vasquez

Copy link
Copy Markdown
Contributor Author

@ax-vasquez

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 yarn.lock in your PR. The repo uses package-lock.json.

Yes, I will be sure to do that once this I have this in a state that's ready for review.

@MikeMcC399

Copy link
Copy Markdown
Collaborator

@ax-vasquez

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.

@ax-vasquez

Copy link
Copy Markdown
Contributor Author

@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!

@MikeMcC399

Copy link
Copy Markdown
Collaborator

@tbiethman

Co-authored-by: Mike McCready <66998419+MikeMcC399@users.noreply.github.com>
@MikeMcC399

Copy link
Copy Markdown
Collaborator

@tbiethman

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/
Click "New project"
Enter a project name, click "Create project"
Under section 3 "Finish CI setup", click GitHub Actions

@tbiethman

Copy link
Copy Markdown

@MikeMcC399 good call, I'll work on getting those updated cloud-side.

Comment thread 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(

@tbiethman tbiethman Aug 31, 2023

Copy link
Copy Markdown

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.

Comment thread README.md Outdated
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
```

#### Branch with PR

Copy link
Copy Markdown

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`

...

Copy link
Copy Markdown
Contributor Author

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.

@tbiethman tbiethman left a comment

Copy link
Copy Markdown

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!

Comment thread README.md Outdated
@MikeMcC399

Copy link
Copy Markdown
Collaborator

@tbiethman

@v6 is now the recommended version, so you can disregard @v5 now

ax-vasquez and others added 4 commits September 1, 2023 11:20

@tbiethman tbiethman left a comment

Copy link
Copy Markdown

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!

Comment thread README.md Outdated
@ax-vasquez

Copy link
Copy Markdown
Contributor Author

@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>

@MikeMcC399 MikeMcC399 left a comment

Copy link
Copy Markdown
Collaborator

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! 👍🏻

@ax-vasquez

Copy link
Copy Markdown
Contributor Author

Awesome, thanks! I committed the change - will merge this on green 👍

@ax-vasquez ax-vasquez merged commit 7215fc1 into master Sep 1, 2023
@ax-vasquez ax-vasquez deleted the armando/fix/CYCLOUD-1447 branch September 1, 2023 17:27
@github-actions

github-actions Bot commented Sep 1, 2023

Copy link
Copy Markdown

🎉 This PR is included in version 6.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants