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

[FEA] changelog generator #599

Merged
merged 12 commits into from
Sep 1, 2020
Merged

[FEA] changelog generator #599

merged 12 commits into from
Sep 1, 2020

Conversation

pxLi
Copy link
Collaborator

@pxLi pxLi commented Aug 21, 2020

fix #573

Generated CHANGELOG.md

example stdout:

.github/workflows/changelog/changelog --token=<GITHUB_PERSONAL_ACCESS_TOKEN>
# STDOUT
Generating changelog ...
Processing pull requests ...
Processing issues ...
Done.
Merged Pull Requests w/o Project:
branch-0.1 #181 Remove INCOMPAT for NormalizeNanAndZero, KnownFloatingPointNormalized https://github.com/NVIDIA/spark-rapids/pull/181
branch-0.1 #191 WindowExec handle different spark distributions https://github.com/NVIDIA/spark-rapids/pull/191
branch-0.1 #195 Support public release for plugin jar https://github.com/NVIDIA/spark-rapids/pull/195
branch-0.1 #206 Change groupID to 'com.nvidia' in IT scripts https://github.com/NVIDIA/spark-rapids/pull/206
branch-0.1 #208 Remove unneeded comment from pom.xml https://github.com/NVIDIA/spark-rapids/pull/208

@pxLi
Copy link
Collaborator Author

pxLi commented Aug 21, 2020

build

1 similar comment
@pxLi
Copy link
Collaborator Author

pxLi commented Aug 21, 2020

build


e.g.
cd spark-rapids/
.github/workflows/changelog/changelog --token=<GITHUB_PERSONAL_ACCESS_TOKEN> --base_refs=branch-0.1,branch-0.2,branch-0.3
Copy link
Collaborator

Choose a reason for hiding this comment

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

might be nice to add change_log is generated for each of the the branches specified.
I assume the assumption here is that we only do 1 release on each branch?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes we are making the assumption that we do one release per branch.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@tgravescs yes, plz take a look at the CHANGELOG.md, this is the rendered markdown preview. Since we separate prs&issues by the release info (github projects), so I try to map the pr&issue back to projects info. And plz let me know if anything else you want

Copy link
Collaborator

Choose a reason for hiding this comment

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

my comment was saying we should update the description of this script to tell user what base_ref really does.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@tgravescs sure thing. I will add a detailed description in upcoming commits

.github/workflows/changelog/changelog Outdated Show resolved Hide resolved
@sameerz sameerz added the build Related to CI / CD or cleanly building label Aug 22, 2020
@pxLi pxLi changed the title [WIP] changelog generator [FEA] changelog generator Aug 25, 2020
@pxLi
Copy link
Collaborator Author

pxLi commented Aug 25, 2020

build

2 similar comments
@pxLi
Copy link
Collaborator Author

pxLi commented Aug 25, 2020

build

@pxLi
Copy link
Collaborator Author

pxLi commented Aug 26, 2020

build

@pxLi
Copy link
Collaborator Author

pxLi commented Aug 26, 2020

build

@pxLi
Copy link
Collaborator Author

pxLi commented Aug 26, 2020

so I added detailed desc in the script. Also there is nan auto-doc-gen workflow within this PR, so the workflow will be triggered each time a PR got merged and create a doc PR for review. If the workflow got triggered multiple times by multiple merged PR, all new commits will be append to the same opened doc PR. please let me know if anything else you want adjust thx! @sameerz @tgravescs @revans2

@jlowe
Copy link
Member

jlowe commented Aug 26, 2020

the workflow will be triggered each time a PR got merged and create a doc PR for review

Please, let's not double the number of PRs. IMO we only need a changelog when we release, so we only need to run the changelog generator as part of the release process.

@sameerz
Copy link
Collaborator

sameerz commented Aug 26, 2020

Can we generate the CHANGELOG.md and auto-merge without doing a build? That way we could generate on some regular cadence (nightly?).

@jlowe
Copy link
Member

jlowe commented Aug 26, 2020

Can we generate the CHANGELOG.md and auto-merge without doing a build? That way we could generate on some regular cadence (nightly?).

I am not a fan of this approach. The changelog results are highly sensitive to proper labeling and assigning to projects in github, and as the first cut showed, this is far from perfect. We need a tool to generate a changelog in preparation for a release, and I believe that changelog is going to need someone to review it for accuracy. Automerging the tool's output without review is asking for trouble like saying some issue is fixed in the release when it was instead closed as won't fix, etc.

@pxLi
Copy link
Collaborator Author

pxLi commented Aug 27, 2020

the workflow will be triggered each time a PR got merged and create a doc PR for review

Please, let's not double the number of PRs. IMO we only need a changelog when we release, so we only need to run the changelog generator as part of the release process.

@jlowe @sameerz actually unlike automerge, the changelog workflow won't merge the PR by itself, and it will keep just one opened doc-gen PR for review, new updates will be committed to the same PR. So team-reviews will be responsible to review the auto-gen doc and merge the PR.

cudf ask to submit PR with changelog update in their commit, but I am also agree with that we could generate the change log when we really do the release.

@pxLi
Copy link
Collaborator Author

pxLi commented Aug 28, 2020

build

@pxLi
Copy link
Collaborator Author

pxLi commented Aug 28, 2020

so I just pushed commits to disable the workflow by rename .github/workflows/{changelog.yml => changelog.yml.disabled}, we could easily turn this on if things changed in the future,
also I removed generated CHANGELOG.md file in this PR, so people could just use the tool to generate changelog when doing release.

jlowe
jlowe previously approved these changes Aug 28, 2020
Copy link
Member

@jlowe jlowe left a comment

Choose a reason for hiding this comment

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

we could easily turn this on if things changed in the future

I personally would never want to double our commit load and clutter our commit history with a bunch of changelog-only PRs. If for some reason we need a changelog update on every PR merged, I'd rather see a hook that does the changelog update as part of the PR commit rather than a separate PR that's auto-merged. It's simply a lot less traffic and noise. In that scenario we're not doing a full changelog generation but a targeted changelog edit which is a very different workflow and script.

This looks OK to me other than I don't think we'll ever use the workflow setup, so I'm dubious on the utility of checking in dead code. Also it's a bit odd to "hide" the script we expect someone to manually run as part of building the release in the .github/workflows directory. I'd rather see this script under a top-level dev/ or scripts/ directory (or maybe even next to the documentation on how to release which should be under the docs/dev/ directory), but that's not a blocker for me. Curious what others think.

@pxLi
Copy link
Collaborator Author

pxLi commented Aug 31, 2020

build

@pxLi
Copy link
Collaborator Author

pxLi commented Aug 31, 2020

we could easily turn this on if things changed in the future

I personally would never want to double our commit load and clutter our commit history with a bunch of changelog-only PRs. If for some reason we need a changelog update on every PR merged, I'd rather see a hook that does the changelog update as part of the PR commit rather than a separate PR that's auto-merged. It's simply a lot less traffic and noise. In that scenario we're not doing a full changelog generation but a targeted changelog edit which is a very different workflow and script.

This looks OK to me other than I don't think we'll ever use the workflow setup, so I'm dubious on the utility of checking in dead code. Also it's a bit odd to "hide" the script we expect someone to manually run as part of building the release in the .github/workflows directory. I'd rather see this script under a top-level dev/ or scripts/ directory (or maybe even next to the documentation on how to release which should be under the docs/dev/ directory), but that's not a blocker for me. Curious what others think.

Ok, I removed the workflow-related code and move the script to the scripts/ folder.
Let's make it simple to keep it just as a script for people to use.

We could add a post-commit hook in the future if someone really need some automation 👍

Copy link
Member

@jlowe jlowe left a comment

Choose a reason for hiding this comment

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

Can the name of this script be changed to something like generate-changelog? Otherwise it looks like it could be a changelog document for the scripts directory.

scripts/changelog Outdated Show resolved Hide resolved
@pxLi
Copy link
Collaborator Author

pxLi commented Aug 31, 2020

build

scripts/generate-changelog Outdated Show resolved Hide resolved
@pxLi
Copy link
Collaborator Author

pxLi commented Sep 1, 2020

build

@pxLi
Copy link
Collaborator Author

pxLi commented Sep 1, 2020

build

1 similar comment
@pxLi
Copy link
Collaborator Author

pxLi commented Sep 1, 2020

build

@pxLi pxLi merged commit 78f2519 into NVIDIA:branch-0.2 Sep 1, 2020
@jlowe jlowe added the feature request New feature or request label Sep 1, 2020
nartal1 pushed a commit to nartal1/spark-rapids that referenced this pull request Jun 9, 2021
* changelog generator

Signed-off-by: Peixin Li <[email protected]>
nartal1 pushed a commit to nartal1/spark-rapids that referenced this pull request Jun 9, 2021
* changelog generator

Signed-off-by: Peixin Li <[email protected]>
tgravescs pushed a commit to tgravescs/spark-rapids that referenced this pull request Nov 30, 2023
[auto-merge] bot-auto-merge-branch-22.10 to branch-22.12 [skip ci] [bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Related to CI / CD or cleanly building feature request New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEA] Create a Change Log
5 participants