I'm marking it as draft until I allocate some time to improve this, probably next week.

Some context:

• For consistency, I tried to follow as much as possible the structure of https://github.com/decidim/decidim/blob/721fb18dfbb131f5f40bcc1b717447ac418a1d3b/bin/changelog_generator. We could probably refactor and remove a bit of duplication (get_data_for_pr method is the same, get_pr_data method is 90% the same). Also, I'd like to move the GH PAT to an ENV var as it's too noisy to have in the command itself, but I've left it there because of consistency.
• As this is directly working with GitHub and git, it could be tricky to test. Also, I don't know if it's even necessary, as it's an internal thing (only for maintainers).
• There's a bug related to the quotation marks and the shell on the title, but I didn't have time yet to solve it.
• It has gh as a dependency and we could probably do it with curl, but for now it's good enough (at least for my use case)

Now I'm happier with the current solution. Regarding the last set of problems:

For consistency, I tried to follow as much as possible the structure of 721fb18/bin/changelog_generator.

On the last week or so there was a big refactor of this file (#9231), so I only keep consistency for how the script is called.

We could probably refactor and remove a bit of duplication (get_data_for_pr method is the same, get_pr_data method is 90% the same).

Moved it to Decidim::GithubManager::Querier and applied it to changelog_generator

Also, I'd like to move the GH PAT to an ENV var as it's too noisy to have in the command itself, but I've left it there because of consistency.

This is still in the script invocation as I'm not fully convinced if this is a good idea. I think the invocation will be cleaner, you'd not leave secrets in your shell history, and this should be configured only one time, but I'd like to hear another opinion about this.

As this is directly working with GitHub and git, it could be tricky to test. Also, I don't know if it's even necessary, as it's an internal thing (only for maintainers).

I've done it already by creating a new repository and mocking up the external calls.

There's a bug related to the quotation marks and the shell on the title, but I didn't have time yet to solve it.

I think this should be fixed as we're not using curl and backticks for this anymore, but I'm not 100% sure as I didin't found any backport with these kind of characters in the last set of PRs to backports. It shouldn't be difficult to fix.

It has gh as a dependency and we could probably do it with curl, but for now it's good enough (at least for my use case)

This is solved already, we're using Faraday instead of gh/curl. Related to dependencies, we could probably use https://github.com/ruby-git/ruby-git instead of system's git, but I prefer to not add a new gem dependency for this, and git's CLI should be available in the maintainers local machine.

This is ready to be reviewed @ahukkanen

This is still in the script invocation as I'm not fully convinced if this is a good idea. I think the invocation will be cleaner, you'd not leave secrets in your shell history, and this should be configured only one time, but I'd like to hear another opinion about this.

Yep this was also something I was thinking when I tried the change log generator before. I'd pass it through ENV vars but I think for the time being we can live with that.

You can also prevent the command being saved to the local history by adding a single space before the command you run. But this is of course up to the users and users make mistakes.

As this is directly working with GitHub and git, it could be tricky to test. Also, I don't know if it's even necessary, as it's an internal thing (only for maintainers).

I've done it already by creating a new repository and mocking up the external calls.

I've read through the code and everything seems fine for me but I'll also test this in actual use later, after which I'll give my actual review.

I think this should be fixed as we're not using curl and backticks for this anymore, but I'm not 100% sure as I didin't found any backport with these kind of characters in the last set of PRs to backports. It shouldn't be difficult to fix.

There are such PRs like #9330 which have the single quote character in their titles. But I'm not sure if there's any better way to handle this because if we changed it to double quotes or backticks, there could be also titles that have those characters in them.

In CSV quotation marks are escaped with doubling the quotes contained in the string but because the titles are meant to be read by humans, I would say we can live with that.

I needed to enable this scope for the personal access token for the PR creation to work correctly:

I was a bit confused how to get it working but after enabling this scope, it works fine.

I also have few improvement ideas for this that I'll post tomorrow.

I needed to enable this scope for the personal access token for the PR creation to work correctly:

I was a bit confused how to get it working but after enabling this scope, it works fine.

This is in the help text:

Copied from the bin/changelog_generator:

I already had it configured for that, so I didn't remember to add it in the PR description. Sorry.

Having the screenshot is much easier to see, maybe adding it at https://docs.decidim.org/en/develop/releases#_producing_the_changelog_md? Or we could start a new section in the docs (Develop -> Maintainers) to start documenting (the Releases page would be there for sure).

This is in the help text:

decidim/bin/backporter

Line 23 in 8ddf57a

You'll need to create one with `public_repo` access.

Copied from the bin/changelog_generator:

decidim/bin/changelog_generator

Line 19 in bbe6722

You'll need to create one with `public_repo` access.

I already had it configured for that, so I didn't remember to add it in the PR description. Sorry.

Having the screenshot is much easier to see, maybe adding it at https://docs.decidim.org/en/develop/releases#_producing_the_changelog_md? Or we could start a new section in the docs (Develop -> Maintainers) to start documenting (the Releases page would be there for sure).

Oh sorry I missed that since I just followed the instructions in the PR (when I didn't see that help message).

Men don't need any instructions with technical stuff/construction projects, right? 🤷‍♂️

@andreslucena Let me know when this is ready for re-review.

I wanted to try it out once more before sending it back to you, and I've just done that Thurrsday, so It's ready to be reviewed again @ahukkanen.

The main change to what you've seen before is that I migrated to Thor as I was arriving to the place where ARGV comes painful.

I tried to give it a spin to the use case that you were proposing, but I don't see it clear, so let's merge this as it's now. Then you can propose whatever improvement on the use cases/feature set that you feel in another PR

This can be additional feature for the future but I noticed that the backport commit messages contain reference to the original PR as well as the backport PR in the commit history:

This is basically because the cherry-pick commit message remains the same as the merged commit and when the backport PR is merged at GitHub, it automatically adds the PR ID at the back of the message unless we manually remove it from there.

I don't see this a problem but more aesthetic issue. We could also programmatically remove the original PR ID from the commit message's title with the following sequence before pushing the backport branch to remote (as an example trial of backporting 9482):

git checkout release/0.27-stable
git checkout -b backport/9482
git cherry-pick 6801ad158cd4752852dd5014f73864de5e05d970
git log --pretty=format:"%B" -1 > backport-message.txt
sed -i -r '1s/(.*) \(#[0-9]+\)$/\1/' backport-message.txt
git commit --amend -F backport-message.txt
rm backport-message.txt

Some of that can be probably simplified with some Ruby code but just to add here as a note how to do that. What it is doing is on top of the cherry-pick:

• Printing the commit message in a temp file (in Ruby, you could just store that)
• Removing the end part that contains parentheses and the PR ID from the first line of the commit message, i.e. the title (could be done also in Ruby)
• Amending the latest commit message with the modified message
• Removing the temp file after amending the commit message

We could also do the message modification in Ruby and then pass it to the amend command as follows:

message = `git log --pretty=format:"%B" -1`
message = message.lines[0].gsub!(/ \(#[0-9]+\)$/, "").concat(*message.lines[1..-1])
message.gsub!('"', '"\""') # Escape the double quotes for bash as they are the message delimiters

`git commit --amend -m "#{message}"`

As said, not a problem. It's an additional feature for the future.

This can be additional feature for the future but I noticed that the backport commit messages contain reference to the original PR as well as the backport PR in the commit history

It looks like I didn't catch that as I'm using the GH extension Refined GitHub.

I've applied and checked locally your fix. I preferred the ruby version, as shell scripting is more difficult to maintain and understand. This will be applied to the next set of PRs, the current one had still this bug.

Now it's ready to be reviewed again. Of course, the best way would be to try this with another set of PRs to fix, or if you prefer, you can close one of the current open backports PR and rerun it against that PR/branch. In that case, you'll also need to delete the remote branch too.

I've applied and checked locally your fix. I preferred the ruby version, as shell scripting is more difficult to maintain and understand. This will be applied to the next set of PRs, the current one had still this bug.

Now it's ready to be reviewed again. Of course, the best way would be to try this with another set of PRs to fix, or if you prefer, you can close one of the current open backports PR and rerun it against that PR/branch. In that case, you'll also need to delete the remote branch too.

I'm not sure if you created all the backports with the latest version but there was still duplicate IDs in the commit message titles:

That said, I have just tried this locally with #9553 and looking at that PR, the commit history looks correct without the original PR ID:

Also when opening the merging section in that PR, I see the commit message title being generated correctly:

So as far as the feature itself goes, this seems to be fixed in the backported script itself. In case this still keeps happening in the future, the problem is probably elsewhere and we need to revisit this then with further invetigation.

I'm not sure if you created all the backports with the latest version but there was still duplicate IDs in the commit message titles:

Yes, that's expected. That's what I meant with

This will be applied to the next set of PRs, the current one had still this bug.

I thought I made that the other day, but I was wrong. Sorry about that