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: Allow for passing custom merge target #838

Merged
merged 1 commit into from
Feb 25, 2022
Merged

feat: Allow for passing custom merge target #838

merged 1 commit into from
Feb 25, 2022

Conversation

kamilogorek
Copy link
Contributor

@kamilogorek kamilogorek commented Feb 17, 2022

See getsentry/craft#355 for more details.

Copy link
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

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

renders a new field in the publish action to type in? Can it be prefiled with the repo's main branch name?

@kamilogorek
Copy link
Contributor Author

Yes, it renders the new field if configured in the target repo.

It cannot be prefilled because this repo, nor action-prepare-release has any knowledge about the target repo, those it doesn't know its default branch name. It can be master, it can be main, but we don't know that afaik. Only craft knows that when you run it.

@BYK
Copy link
Member

BYK commented Feb 17, 2022

It cannot be prefilled because this repo, nor action-prepare-release has any knowledge about the target repo, those it doesn't know its default branch name.

action-prepare-release knows what repo it runs in with a GITHUB_TOKEN so it should be able to determine the main branch?

.github/workflows/publish.yml Outdated Show resolved Hide resolved
@kamilogorek
Copy link
Contributor Author

I’ll research that. It would be nice, as we could remove default parsing override in publish repo, as the value would be always there.

Comment on lines 9 to 12
const mergeTargetParser = /^Merge target: (?<merge_target>.+)$/m;
let { merge_target } = context.payload.issue.body.match(
mergeTargetParser
).groups;
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 should be optional until everybody gets to the latest version of action-prepare-release. Otherwise you'll have lots of failures.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, forgot about the fallback for destructuring. Because other than this, merge_target can be undefined, as it should evaluate to empty env variable, thus being skipped in cli call. (not that I haven't test any of this yet, as its not decided what we'll do with the original issue just yet).

@kamilogorek
Copy link
Contributor Author

I cannot find any way to use dynamic value for inputs.default in composite gh action :(

I'm also starting to wonder if there is even a point doing this, as craft itself handles default branch 🤔

@BYK
Copy link
Member

BYK commented Feb 18, 2022

I'm also starting to wonder if there is even a point doing this, as craft itself handles default branch 🤔

I'd just leave it empty so Craft does "the right thing"™️

@mitsuhiko mitsuhiko merged commit 88b505c into getsentry:main Feb 25, 2022
@kamilogorek kamilogorek deleted the merge-target branch February 25, 2022 15:00
@jan-auer
Copy link
Member

jan-auer commented Mar 1, 2022

@kamilogorek would be great if we add a section to the readme on how to publish from/to a non-default branch similar to how we describe "CalVer".

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.

5 participants