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

Improved version of source_changelog #541

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

j-rivero
Copy link
Contributor

@j-rivero j-rivero commented Oct 27, 2021

While reading the latest changes to github product cli I just noticed that they have an script to generate changelog entries: https://github.com/cli/cli/blob/4e219a9c8f4fb2b38c2b9ff001a39729b58d61f8/script/changelog.

In trunk (hello subversion) branch in the repository they have changed it to call github API, which should probably be the way to go for us too.

Meanwhile I modified their script a little bit, I think is ready to test. A quick launch on ign-tools repository seems fine (note I disabled the check for external contributors, openrobotics emails are filtered out):

## Ignition Tools 1.x (2021-10-27)

1. Fix windows colcon build
    * [Pull request #65](https://github.com/ignitionrobotics/ign-tools/pull/65)
    * A contribution from: Steve Peters <[email protected]>

1. Fix use of Backward on macOS and add a unit test 
    * [Pull request #67](https://github.com/ignitionrobotics/ign-tools/pull/67)
    * A contribution from: Addisu Z. Taddese <[email protected]>

1. Support colcon in windows CI
    * [Pull request #66](https://github.com/ignitionrobotics/ign-tools/pull/66)
    * A contribution from: Steve Peters <[email protected]>

1. Add flag to get standard-compliant exception handling
    * [Pull request #68](https://github.com/ignitionrobotics/ign-tools/pull/68)
    * A contribution from: Addisu Z. Taddese <[email protected]>

Note: to review it, just look at the plain raw file, diff make not sense here.

@scpeters
Copy link
Contributor

I like the automatic tag detection, that's a definite improvement. I think it could use some more documentation about the git magic used to detect the tag, but I think it is a great idea.

One difference I noticed is how it treats merge-forward pull requests. I just tested it with sdf10 as I prepare for a 10.7.0~pre1 prerelease, and it listed two merge-forward pull requests in the changelog but didn't list the commits that were merged forward. With this branch it shows 6 new Changelog entries, while the master branch version of this script generates 40 entries. I realize that there's been recent discussion about whether to show features repeatedly in different major versions of the changelog, and I can't remember what the result of that discussion was, but I think we should consider how we want this script to handle merge commits.

@scpeters
Copy link
Contributor

I started with this branch for gazebosim/sdformat#799 but finished by hand to include the forward-port PRs

@chapulina
Copy link
Contributor

Oh I totally forgot about this PR!

it listed two merge-forward pull requests in the changelog but didn't list the commits that were merged forward.

Ah yeah we should have all the commits that have been merged forward

@j-rivero
Copy link
Contributor Author

Oh I totally forgot about this PR!

it listed two merge-forward pull requests in the changelog but didn't list the commits that were merged forward.

Ah yeah we should have all the commits that have been merged forward

Found some time to make the changes. I tried to use the same git log approach that we currently have to reflect merges in the same way. d577f2a.

Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

Nice! I like it how it automatically picks up the tag.

There's still some issue with the merge commits though. If I run this on ign-gazebo right now, I only get the forward-port PR, but there are many other commits in the diff:

gazebosim/gz-sim@ignition-gazebo6_6.9.0...ign-gazebo6

printf "1. %s\n" "$pr_desc"
printf " * [Pull request #%s](%s/pull/%s)\n" "$pr_num" "$current_origin_remote" "$pr_num"
if [[ "${author_email/@openrobotics.org}" == "${author_email}" ]] && \
[[ "${author_email/@osrfoundation.org}" == "${author_email}" ]] && \
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure about excluding us, we need love too ❤️

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.

3 participants