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

Only consider commits before commitish, if provided #1058

Closed
wants to merge 1 commit into from

Conversation

mikeroll
Copy link
Contributor

@mikeroll mikeroll commented Feb 7, 2022

Resolves #1053

When searching for commits to be included in the release notes, if commitish is supplied, it is first resolved to a commit sha. That commit sha is then used a starting point for the graphql query. This allows us to generalize the process and support both "static" (commits, tags) and "dynamic" (branches) commitish values.
This does not affect target_commitish of the release, which will always be the value provided, with no modifications.

Demo repository is here:
https://github.com/mikeroll/release-drafter-1053/releases
https://github.com/mikeroll/release-drafter-1053/actions
Each of the 4 actions correspond to release-drafter configured differently. Added @jetersen to contributors if you want to play around, any of those actions may be restarted to see the different behaviours.
no-commitish - uses the repository's default branch, as before. The action is for an old commit, but even if re-run it will include all the later commits in the release, as before.
commitish-as-branch - same as above, with the branch specified explicitly.
commitish-as-sha{,-2} - the sha of the commit being built (${{ github.sha }}) is set as the release target (already worked before) and only the commits up to and including this one are included in the release notes, i.e. the build for commit 3 does not add commit 4.

@mkurz
Copy link
Contributor

mkurz commented Feb 8, 2022

I think you completely misused the commitish config. The commitish config solely exists to set the target_commitish property of a github release. So basically that property means "this release was done from this ref" (ref usually being a branch).
Now what you do is you take this config, use it for something completely different. If you create a release with this pull request applied and use the commitish config for your "up-to-commit" concept, this will end up creating a release whose meta data ( = target_commitish property which got set via release drafters' commitish config) are not correct. I don't even know if GitHub accepts a target_commitish which is not a branch but just some sha.
Do you know what I mean? If not please have a look in the code what the commitish config is used for right now, it is NOT used to determine anything which release or commit to use. It just gets passed through to add meta data to a GitHub release.

@mikeroll
Copy link
Contributor Author

mikeroll commented Feb 8, 2022

Now what you do is you take this config, use it for something completely different. If you create a release with this pull request applied and use the commitish config for your "up-to-commit" concept, this will end up creating a release whose meta data ( = target_commitish property which got set via release drafters' commitish config) are not correct.

Please see the diff - nothing about releases there, commitish is only resolved to a sha when searching for commits to include.
Having commitish: release-branch will still set target_commitish: "release-branch" on the release because that's what you asked, no magic.

The actual change is this: if I want my release/tag on release-branch, might as well search and include commits which are on that same release-branch (this PR), and not on the repository's default branch (current behaviour), no? If release-branch is the default branch - nobody notices, but if it's not - stuff breaks down as you described in #1061, simply because it's looking in the wrong place.

If I could do after: "release-branch 0" in the GraphQL query, I would, but it needs a commit hash so I give it one - of commit that release-branch points to.

I don't even know if GitHub accepts a target_commitish which is not a branch but just some sha.

The definition of commitish is roughly "anything that points to a specific commit" as per Git, and Github seems to play along with that as far as I looked, and yes target_commitish also works perfectly.

EDIT: Tried to reflect all this in the PR description, as it was admittedly lacking.

@mikeroll
Copy link
Contributor Author

mikeroll commented Feb 8, 2022

I was actually wrong - the commit search is fine right now, and the PR does not fix anything regarding that. It just adds the possibilty to stop at a certain commit, if that's what's set as commitish.

Also the implementation could be simpler, will follow up shortly.

@mikeroll
Copy link
Contributor Author

Closing in favor of #1065

@mikeroll mikeroll closed this Feb 10, 2022
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.

Only consider commits that come before commitish?
2 participants