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

feat: Use hybrid approach depending on runner #263

Closed
wants to merge 29 commits into from

Conversation

andreiborza
Copy link
Member

@andreiborza andreiborza commented Feb 24, 2025

Background

This PR changes the architecture of the GitHub Action.

Prior to release 1.10.0, the GitHub Action was only available for Linux based GitHub Action runners, limiting the reach of our Action.

With release 1.10.0 the architecture was switched from being a Docker based GitHub Action to a composite GitHub Action. While the bulk of the JavaScript that runs was pre-built, the key piece of the action (@sentry/cli) was installed on each invocation of the action. This takes a long time, but allows us to support a variety of GitHub runners, for example runners running on macOS and Windows.

The hit to execution time (in some cases the action ran 2x as long as before 1.10.0) and the lack of control over the runner environment and what packages are available (e.g. node, npm, yarn...) meant that many users could not use this new architecture out of the box.

On the other hand, we also want to continue supporting non-Linux GitHub Action runners.

Therefore the architecture has been changed to a hybrid approach.

  • For Linux based runners, a pre-published Docker image will be downloaded and executed. This should match behavior prior to 1.10.0
  • For non-Linux based runners, the composite action architecture will run similarly to 1.10.x with a difference: node will now always be set up using actions/setup-node regardless of a node version already existing to ensure the action can install @sentry/cli and run the JavaScript script.

Workflow

The development workflow has thus slightly changed as well:

  1. Create a branch
  2. Run yarn set-docker-tag-from-branch to set a docker tag based on your git branch name. This is important so that the action gets its own Docker image, allowing you to test the action.
  3. Make changes
  4. If possible, add unit and E2E tests (inside .github/workflows/build.yml)
  5. Run yarn install to install deps
  6. Run yarn build to build the action
  7. Commit the changes and the build inside dist/

If you forget to run yarn set-docker-tag-from-branch the repo's pre-commit hooks will do it for you and fail the commit.
Just add the changes to staging and commit again.

@andreiborza andreiborza reopened this Feb 24, 2025
@andreiborza andreiborza requested review from Lms24 and removed request for Lms24 February 24, 2025 14:10
@andreiborza andreiborza marked this pull request as draft February 24, 2025 15:16
@andreiborza andreiborza force-pushed the ab/multiarch-docker branch 4 times, most recently from 934b5e8 to 8350bae Compare February 25, 2025 11:04
@andreiborza andreiborza force-pushed the ab/multiarch-docker branch 2 times, most recently from 9d7df48 to e1429b4 Compare February 25, 2025 12:15
@andreiborza andreiborza force-pushed the ab/multiarch-docker branch 2 times, most recently from 24f9218 to f42dbcf Compare February 25, 2025 12:58
@andreiborza andreiborza force-pushed the ab/multiarch-docker branch 2 times, most recently from 3e7d7a0 to 8e364cd Compare February 25, 2025 13:14
@andreiborza andreiborza marked this pull request as ready for review February 25, 2025 13:32
@andreiborza andreiborza requested a review from Lms24 February 25, 2025 13:32
@Lms24
Copy link
Member

Lms24 commented Feb 25, 2025

I didn't yet look at the code changes (will do so right after) but I just had an idea:

After you create a branch ALWAYS run yarn set-docker-tag-from-branch.
This is very important.

This immediately raised a red flag tbh because I'm quite sure that I personally would forget this, even after having reviewed this PR. So chances are high someone else would forget this, too.

Can we use the post-checkout git hook to ensure this is done automatically?

@andreiborza
Copy link
Member Author

I didn't yet look at the code changes (will do so right after) but I just had an idea:

After you create a branch ALWAYS run yarn set-docker-tag-from-branch.
This is very important.

This immediately raised a red flag tbh because I'm quite sure that I personally would forget this, even after having reviewed this PR. So chances are high someone else would forget this, too.

Can we use the post-checkout git hook to ensure this is done automatically?

I added pre-commit hooks for formatting, linting and automatically setting the docker tag based on branch name, all of which are skipped in the case of committing the master tag back for master CI runs.

Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

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

Nice, thanks for adding the pre-commit hooks!

@andreiborza
Copy link
Member Author

Please don't merge this PR yet. I want to ask some users to give it a spin.

@andreiborza
Copy link
Member Author

andreiborza commented Feb 27, 2025

This PR will stay open for a week until March 6th to give users, who graciously helped out testing the PR, a chance to migrate off of it, after which it will be closed in favor of: #265

@andreiborza
Copy link
Member Author

This PR has been duplicated and merged (see #265).

@andreiborza andreiborza closed this Mar 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants