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
Unify commit search and release targeting around commitish
/ref
#1065
Unify commit search and release targeting around commitish
/ref
#1065
Conversation
a88b00b
to
135ca7a
Compare
commitish
commitish
/ref
88a0793
to
c9b1cd8
Compare
How would it work when release drafting is run on tag? ie. https://github.com/release-drafter/release-drafter/blob/master/.github/workflows/release.yml Perhaps target_commitish should default to default branch? depending on the above. |
@jetersen I'm not sure I completely understand the "on tag" case. Say, you push a |
I expect the release notes to be drafted between previousRelease and default branch. However by this change it would be previousRelease ... newTag reference. Your basically changing the default behavior, I only wonder if it changes the overall behavior. |
I did some more research and updated the tables in the PR description. But the thing is, it was always like that:
You may have had the impression that it's The only difference would be the |
Than some release flows to break because it errors? release-drafter/.github/workflows/release.yml Lines 55 to 60 in 257fa0e
|
No, I meant I'll update this PR so that |
Other question still applies would be more correct to use: |
It doesn't matter, because first the tag is pushed, then the release is created for that tag by
|
Handled the tags with the last commit, should be ready now. |
c59742d
to
9041335
Compare
@jetersen @mikeroll I think this code breaks default functionality: I assume that
This will never ever match existing releases as it tries to match Update - nvm, this seems to be the issue: #1081 |
Just a FYI this is actually using a regex I guess we could avoid using a regex in this case and simple filter on if |
ref was renamed
I think that would be better in this case, as releases.filter((r) =>
targetCommitish.match(`/${r.target_commitish}$`)
|| targetCommitish === r.target_commitish
) |
My bad, I skimmed over this one when introducing the change. @robbinjanssen In case you want a real fast "free" fix: the fact that it worked for you before indicates that it was always As for the actual fix, this should probably be (pseudocode, don't know the js equivalent yet):
|
ref was renamed
ref was renamed
ref was renamed
Below is how it is currently.
master
is the default branch.ref
is the ref that release-drafter runs for (GITHUB_SHA
for Actions or the ref from the webhook payload for App).commitish
is the value provided in the config or action inputs,target_commitish
is what effectively gets set in the release.ref
commitish
target_commitish
refs/heads/master
master
$prev_release..master
refs/heads/master
master
master
$prev_release..master
refs/heads/master
a-branch
a-branch
$prev_release..master
refs/heads/a-branch
master
$prev_release..a-branch
refs/heads/a-branch
a-branch
a-branch
$prev_release..a-branch
refs/heads/a-branch
ab1e2fc3
ab1e2fc3
$prev_release..a-branch
refs/tags/v1.0.0
<ignored>
$prev_release..v1.0.0
refs/tags/v1.0.0
v1.0.0
<error>
$prev_release..v1.0.0
)I'd argue that 3, 4, 6 are outright broken (3 is a rather unrealistic use case IMO, but it's there for completeness' sake):
commitish
. Running on a non-default branch still sets the release target tomaster
, while including commits for that branch.commitish
does fix the incorrect commit range, but why is this required to make the thing work?This PR makes sure that what you see in the notes is what gets released. The target for both can be reliably controlled via
commitish
if you need a custom workflow or just want a more explicit configuration.If not supplied,
ref
aka "current branch" is used instead and is also consistent for both.Fixing 6 also resolves #1053.
ref
commitish
target_commitish
refs/heads/master
master
$prev_release..master
refs/heads/master
master
master
$prev_release..master
refs/heads/master
a-branch
a-branch
$prev_release..a-branch
refs/heads/a-branch
a-branch
$prev_release..a-branch
refs/heads/a-branch
a-branch
a-branch
$prev_release..a-branch
refs/heads/a-branch
ab1e2fc3
ab1e2fc3
$prev_release..ab1e2fc3
refs/tags/v1.0.0
<ignored>
$prev_release..v1.0.0
refs/tags/v1.0.0
v1.0.0
<error>
$prev_release..v1.0.0
)Regarding 8: providing
commitish
that is a tag could teoretically work, by isn't supported by github. Didn't work before, not adding any magic to make it work now.