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

ref(publish): Add sentry-wizard/1.x branch to target-repo-branch step #3950

Merged

Conversation

Lms24
Copy link
Member

@Lms24 Lms24 commented Jun 10, 2024

Adds the 1.x branch of the sentry-wizard repo to be one of the branches where we can take the craft config from. The config diverged between 1.x and 3.x (latest) and we need to cut a 1.x release. So I'd like to opt this branch into the mechanism we set up for the JS SDK repo.

@@ -46,6 +46,7 @@ jobs:
fromJSON(steps.inputs.outputs.result).repo == 'sentry-migr8' && fromJSON(steps.inputs.outputs.result).merge_target == 'tmp-merge-target' ||
fromJSON(steps.inputs.outputs.result).repo == 'sentry-javascript' && fromJSON(steps.inputs.outputs.result).merge_target == 'v7' ||
fromJSON(steps.inputs.outputs.result).repo == 'sentry-javascript' && fromJSON(steps.inputs.outputs.result).merge_target == 'master' ||
fromJSON(steps.inputs.outputs.result).repo == 'sentry-wizard' && fromJSON(steps.inputs.outputs.result).merge_target == '1.x' ||
Copy link
Member

Choose a reason for hiding this comment

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

this branch does not have branch protection

Copy link
Member Author

Choose a reason for hiding this comment

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

Stephanie just updated protection rules for 1.x 🙏

Copy link
Member

Choose a reason for hiding this comment

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

the protection rule does not require CI to pass -- only approval. should it have some sort of automated quality checks?

Copy link
Member Author

@Lms24 Lms24 Jun 10, 2024

Choose a reason for hiding this comment

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

It seems it there is/was some CI for 1.x (v1 is two years old by now, we're on 3.x right now) but I'm not sure why CI isn't running for 1.x at the moment. It should at least run for a PR but apparently it doesn't. Any ideas?

I just want to get a patch release out because a user still relying on v1 and they contributed a small fix themselves. I think we already achieved branch protection in the sense that we require approval before merging. Is this enough?

Copy link
Member

Choose a reason for hiding this comment

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

actually taking a step back -- seems like we're going through a ton of work for something we don't support. should we just remove the docs showing @1 and claim this is unsupported territory?

Copy link
Member Author

Choose a reason for hiding this comment

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

I would ask that we focus on the important aspects here which is to enable obtaining the craft config of v1 for v1 releases instead of the recent v3 one. The configs diverged. For instance we added the brew target in the meantime and I definitely don’t want to release 1.x to brew. The alternative would be to hack around in the v3 craft config temporarily or get one of the ~4 people in the company with the necessary permissions to uncheck the irrelevant check boxes in the publish issue but should we ever need to do another v1 release, we’re right back where we started.

should we just remove the docs showing @1 and claim this is unsupported territory?

I’m trusting the Mobile SDK team that v1 is mentioned for good reason in docs. As we can see, users do rely on it, AFAICT for older RN project setups where the v3 wizard wouldn’t work anymore. Furthermore, again, a user is relying on v1 and they contributed the fix themselves. Releasing should be the easy part here.

seems like we’re going through a ton of work

Frankly, most of what’s necessary for this was already done and took me ~30 minutes. Using the correct craft config is last piece of the puzzle and it’s by now taking more time than everything else combined :)

I do agree, this is not a p0 change but again, let’s just get this out to improve DX for our users and avoid spending more time on this than necessary.

Copy link
Member

@HazAT HazAT left a comment

Choose a reason for hiding this comment

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

For the sake of unblocking our users, let's just merge this - backporting fixes shouldn't be a huge deal.
I recognize the solution is not ideal so next time, let's maybe move all the repos + branches into a config file that we use in the action.

@HazAT HazAT merged commit 869a08c into getsentry:main Jun 12, 2024
1 check passed
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.

3 participants