Unify commit search and release targeting around commitish/ref

[mikeroll]

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	commits included in notes
1	refs/heads/master		master	$prev_release..master
2	refs/heads/master	master	master	$prev_release..master
3	refs/heads/master	a-branch	a-branch	$prev_release..master
4	refs/heads/a-branch		master	$prev_release..a-branch
5	refs/heads/a-branch	a-branch	a-branch	$prev_release..a-branch
6	refs/heads/a-branch	ab1e2fc3	ab1e2fc3	$prev_release..a-branch
7	refs/tags/v1.0.0		<ignored>	$prev_release..v1.0.0
8	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):

• What you are releasing does not match what's in the release notes!
• You can't trust the default of not providing commitish. Running on a non-default branch still sets the release target to master, while including commits for that branch.
• Providing the matching 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	commits included in notes
1	refs/heads/master		master	$prev_release..master
2	refs/heads/master	master	master	$prev_release..master
3	refs/heads/master	a-branch	a-branch	$prev_release..a-branch
4	refs/heads/a-branch		a-branch	$prev_release..a-branch
5	refs/heads/a-branch	a-branch	a-branch	$prev_release..a-branch
6	refs/heads/a-branch	ab1e2fc3	ab1e2fc3	$prev_release..ab1e2fc3
7	refs/tags/v1.0.0		<ignored>	$prev_release..v1.0.0
8	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.

[jetersen]

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.

[mikeroll]

@jetersen I'm not sure I completely understand the "on tag" case. Say, you push a v5.18.2 now, what do you expect to happen?

[jetersen]

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.

[mikeroll]

I did some more research and updated the tables in the PR description. But the thing is, it was always like that:

previousRelease ... newTag

You may have had the impression that it's ... master instead because you were always tagging HEAD@master?

The only difference would be the target_commitish value, but even that is theoretical, because target_commitish is ignored when the tag already exists.
In fact, I just discovered that github rejects target_commitish: <tag> with an error, so I shouldn't even try in this case (and luckily don't have to) - this is the only special detail about tags.

[jetersen]

In fact, I just discovered that github rejects target_commitish: with an error, so I shouldn't even try in this case (and luckily don't have to) - this is the only special detail about tags.

Than some release flows to break because it errors?
Could we consider this a breaking change? As in our release workflow we would have to add commitish: ${{ github.sha }} in with block inside:

release-drafter/.github/workflows/release.yml

Lines 55 to 60 in 257fa0e

	- uses: release-drafter/release-drafter@master
	with:
	version: ${{ steps.version.outputs.version }}
	publish: true
	env:
	GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}

[mikeroll]

No, I meant I'll update this PR so that target_commitish is excluded from request payload if ref/commitish is a tag. Nothing would need to be changed.

[jetersen]

Other question still applies would be more correct to use: commitish: ${{ github.sha }} ? :)

[mikeroll]

It doesn't matter, because first the tag is pushed, then the release is created for that tag by release.yml (am I right?). target_commitish is ignored when creating/updating releases for existing tag. Release notes would be the same either for $last_release..$this_tag or $last_release..$this_sha so I'd say no need to change.

commitish: ${{ github.sha }} is definitely more correct when it's on: { push: { branches: [some-branch] } } though, as this prevents race conditions (what I described in #1053). Tags are "static" so don't have this problem.

[mikeroll]

Handled the tags with the last commit, should be ready now.

[robbinjanssen]

@jetersen @mikeroll I think this code breaks default functionality:

I assume that targetCommitish comes from the config and in my case is: commitish: master with filter-by-commitish: true

  const filteredReleases = filterByCommitish
    ? releases.filter((r) => targetCommitish.match(`/${r.target_commitish}$`))
    : releases

This will never ever match existing releases as it tries to match 'master' with '/master'.

---

Update - nvm, this seems to be the issue: #1081

[jetersen]

@jetersen @mikeroll I think this code breaks default functionality:

I assume that targetCommitish comes from the config and in my case is: commitish: master with filter-by-commitish: true

  const filteredReleases = filterByCommitish
    ? releases.filter((r) => targetCommitish.match(`/${r.target_commitish}$`))
    : releases

Just a FYI this is actually using a regex /master$

I guess we could avoid using a regex in this case and simple filter on if targetCommitish === r.target_commitish

[robbinjanssen]

I think that would be better in this case, as 'master'.match('/master$'); results in null? Maybe it should be something like:

releases.filter((r) => 
  targetCommitish.match(`/${r.target_commitish}$`) 
  || targetCommitish === r.target_commitish
)

[mikeroll]

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 'refs/heads/master'.match('/master$') - not because you specified commitish: master (it had zero effect on this expression) but because you are running the release-drafter action on the master branch. Since commitish now rightfully defaults to GITHUB_REF (refs/heads/master in your case), you may just remove your commitish.

As for the actual fix, this should probably be (pseudocode, don't know the js equivalent yet):

targetCommitish.trimsuffix('refs/heads/') === r.target_commitish.trimsuffix('refs/heads/')