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(publish): Add allowlist for branches to take craft config from #3452

Merged

Conversation

Lms24
Copy link
Member

@Lms24 Lms24 commented Feb 16, 2024

This PR adds a step to the publish.yml workflow that determines which branch of the target repo should be checked out.
The checked out branch will be the one where craft takes its config from.

By default, we always check out the target repo's default branch.

ref #3441

@Lms24 Lms24 requested a review from a team as a code owner February 16, 2024 15:03
Comment on lines 48 to 49
fromJSON(steps.inputs.outputs.result).repo == 'sentry-migr8' && fromJSON(steps.inputs.outputs.result).merge_target == 'main' ||
fromJSON(steps.inputs.outputs.result).repo == 'sentry-migr8' && fromJSON(steps.inputs.outputs.result).merge_target == 'tmp-merge-target'
Copy link
Member Author

@Lms24 Lms24 Feb 16, 2024

Choose a reason for hiding this comment

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

I opted to temporarily add an entry for sentry-migr8 here so that I can test this before we add e.g. the JS SDK repo.

Copy link
Member Author

@Lms24 Lms24 Feb 16, 2024

Choose a reason for hiding this comment

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

For sentry-migr8 it should be fine to cut a few releases as it's still just a hackweek project at the moment
To test, I removed the npm target. We can even clean up the GH release afterwards.

Copy link
Member

Choose a reason for hiding this comment

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

let's add branch protection to that repo just so we're following the process

Copy link
Member Author

Choose a reason for hiding this comment

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

- name: Set target repo checkout branch
# Note: Branches registered here MUST BE protected in the target repo!
if: |
fromJSON(steps.inputs.outputs.result).repo == 'sentry-migr8' && fromJSON(steps.inputs.outputs.result).merge_target == 'main' ||
Copy link
Member

Choose a reason for hiding this comment

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

I think this first one wouldn't be needed since this is the default branch?

Copy link
Member Author

Choose a reason for hiding this comment

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

removed

Copy link
Member

@asottile-sentry asottile-sentry left a comment

Choose a reason for hiding this comment

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

@asottile-sentry asottile-sentry merged commit 348e3a0 into getsentry:main Feb 16, 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.

2 participants