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

Move Browserstack into a seperate workflow #1126

Merged
merged 5 commits into from
Aug 29, 2023
Merged

Conversation

frederikprijck
Copy link
Member

@frederikprijck frederikprijck commented Aug 26, 2023

Changes

Opening this PR as draft as a discussion starter to see if what I'm doing here makes sense and would be something we would want.

The PR separates Browserstack into its own workflow, doing that removes the need for using the pull_request_target trigger on the Build and Test workflow, making it easier to make modifications to that workflow file when needed.

It does keep the pull_request_target and authorize step for the Browserstack workflow to guarantee our secrets are protected by the means of a manual approval step.

Why?

Using pull_request_target does not run workflow changes on PRs as it ensures it always runs in the context of the target branch. For example, I opened a PR to update the NODE_VERSION to 20, but you will notice that it still uses node 18 to execute the PR's workflow. This means that we will only know if it works with node20 on GitHub Actions after merging it to master.

However, the only reason we use pull_request_target is for Browserstack. If we separate Browserstack into its own workflow, we no longer need pull_request_target on our main Build and Test workflow, meaning in that case, the PR I opened would run against node 20 on the PR level, and not after merging.

I'm not yet sure if and how this will work for automating our release, as I know it becomes really complicated when u have separate workflows. But we already have the publish in a separate workflow anyway, so adding a 3rd one doesn't necessarily add additional complexity, I think.

Unrelated, but also Drops --include=dev from npm ci, as we don't need that. But same goes for changes like this. We have no way to identify this works before merging this PR, because of the usage of pull_request_target, which (as mentioned above) is not needed to build our SDK and run its tests.

Checklist

@frederikprijck frederikprijck temporarily deployed to internal August 26, 2023 17:15 — with GitHub Actions Inactive
@frederikprijck frederikprijck temporarily deployed to internal August 26, 2023 17:15 — with GitHub Actions Inactive
@evansims
Copy link
Member

evansims commented Aug 28, 2023

LGTM! Makes perfect sense to split things up this way. A few thoughts:

  1. Be sure to update the checkout event to use the ref property to support pull_request_target (as you correctly reminded me of the other day! 😆)
- uses: actions/checkout@v3
  with:
    ref: ${{ github.event.pull_request.head.sha || github.ref }}
  1. I think I made a mistake with the BrowserStack call for pull_request_target, as github.sha may reference the wrong SHA for that event. You may want to change the line to instead read:
npx concurrently --raw --kill-others --success first "npm:dev" "wait-on http://127.0.0.1:3000/ && browserstack-cypress run --build-name ${{ github.event.pull_request.head.sha || github.ref }}"
  1. I've also identified a few other improvements I've suggested on the Lock repository you might want to consider merging with this PR. For example, we can streamline Dependabot PRs by running those in the internal environment context automatically under the authorize job:
environment: ${{ github.actor != 'dependabot[bot]' && github.event_name == 'pull_request_target' && github.event.pull_request.head.repo.full_name != github.repository && 'external' || 'internal' }}

@frederikprijck frederikprijck temporarily deployed to internal August 29, 2023 08:12 — with GitHub Actions Inactive
@frederikprijck frederikprijck temporarily deployed to internal August 29, 2023 08:12 — with GitHub Actions Inactive
@frederikprijck frederikprijck temporarily deployed to internal August 29, 2023 08:14 — with GitHub Actions Inactive
@frederikprijck
Copy link
Member Author

Thanks, I made the changes you mentioned @evansims . Please take another look.

@frederikprijck frederikprijck marked this pull request as ready for review August 29, 2023 08:14
@frederikprijck frederikprijck requested a review from a team as a code owner August 29, 2023 08:14
@frederikprijck frederikprijck temporarily deployed to internal August 29, 2023 08:19 — with GitHub Actions Inactive
@frederikprijck frederikprijck temporarily deployed to internal August 29, 2023 08:29 — with GitHub Actions Inactive
@frederikprijck frederikprijck temporarily deployed to internal August 29, 2023 08:29 — with GitHub Actions Inactive
@frederikprijck frederikprijck temporarily deployed to internal August 29, 2023 08:32 — with GitHub Actions Inactive
Copy link
Member

@evansims evansims left a comment

Choose a reason for hiding this comment

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

Awesome — LGTM!

@frederikprijck frederikprijck added this pull request to the merge queue Aug 29, 2023
Merged via the queue into master with commit e8e4f2c Aug 29, 2023
22 checks passed
@frederikprijck frederikprijck deleted the feat/rework-ga branch August 29, 2023 22:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants