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.

Sign up for GitHub

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

Jump to bottom

feat(publish): Use craft config from merge target if configured #3451

Closed
wants to merge 2 commits into from
Closed

feat(publish): Use craft config from merge target if configured #3451

wants to merge 2 commits into from

Conversation

Lms24
Copy link
Member

@Lms24 Lms24 commented Feb 16, 2024 •
edited
Loading

This PR does 2 things:

  1. Extract the merge target branch information added to the publish issue in feat: Write craft config branch to issue action-prepare-release#35
  2. Checkout the target repo on the merge target branch if we extracted merge_target from

In a follow up PR, I'll add an allowlist for repos and branches where we can take the config from and fall back if the merge target isn't included in the allowlist.

ref #3441

@Lms24 Lms24 requested a review from a team as a code owner February 16, 2024 13:18
@Lms24 Lms24 force-pushed the lms/feat-craft-config-from-merge-target branch from 6f0b910 to b2335d8 Compare February 16, 2024 13:23
@Lms24
Copy link
Member Author

Lms24 commented Feb 16, 2024

@asottile-sentry can't request a review due to repo permissions I guess. Would you mind taking a look? thanks!

Lms24
Lms24 commented Feb 16, 2024
View reviewed changes
src/modules/details-from-context.js
@@ -1,9 +1,10 @@
async function detailsFromContext({ context }) {
if (!context || !context.payload || !context.payload.issue) {
throw new Error('Issue context is not defined');
throw new Error("Issue context is not defined");
Copy link
Member Author

Choose a reason for hiding this comment

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

formatting change

src/modules/details-from-context.js
Comment on lines +6 to +7
const titleParser =
/^publish: (?:getsentry\/)?(?<repo>[^/@]+)(?<path>\/[\w./-]+)?@(?<version>[\w.+-]+)$/;
Copy link
Member Author

Choose a reason for hiding this comment

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

formatting change

Copy link
Member

Choose a reason for hiding this comment

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

please discard the unrelated formatting changes from your PR so I can review the actual change

src/modules/details-from-context.js
const { requester } = context.payload.issue.body.match(
requesterParser
).groups;
const requesterParser =
Copy link
Member Author

Choose a reason for hiding this comment

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

formatting change

@Lms24 Lms24 mentioned this pull request Feb 16, 2024
Closed
@BYK
Copy link
Member

BYK commented Feb 16, 2024

In a follow up PR, I'll add an allowlist for repos and branches where we can take the config from and fall back if the merge target isn't included in the allowlist.

This should come first for safety. I'd also say (as a next step to the allow list) that it would be better to check whether the branch name is covered by branch protection rules. I think secrets.GH_SENTRY_BOT_PAT token would have access to read other repo settings. If not you can ask @HazAT or others in that chain to add that permission.

@Lms24
Copy link
Member Author

Lms24 commented Feb 16, 2024

Superseded by #3452

@Lms24 Lms24 closed this Feb 16, 2024
@Lms24 Lms24 mentioned this pull request Feb 16, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asottile-sentry asottile-sentry asottile-sentry left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Lms24 @BYK @asottile-sentry