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

build(docs): include full changelog and all contributors #13235

Merged

Conversation

agilgur5
Copy link
Contributor

@agilgur5 agilgur5 commented Jun 22, 2024

Partial fix for #13206 (comment)

Motivation

  • there's a filter on the commits in the changelog generation script, but the CHANGELOG does not mention that

Modifications

  • say "Selected Changes" to explicitly note that it is not full

    • and put a link to the Full Changelog/Full Diff via GH compare view above that
  • include all Contributors to the release, not just the filtered ones

    • also collapse the list of contributors by default with the HTML <details> tag

Verification

I ran the changelog.sh script locally and committed the resulting CHANGELOG.md in this PR.

  • See the rendered commit
  • I also double-checked the entirety of the diff myself

- there's a filter on the commits in the changelog generation script, but the CHANGELOG does not mention that
  - say "Selected Changes" to explicitly note that
  - and put a link to the Full Changelog/Full Diff via GH compare view above that

- include all Contributors to the release, not just the filtered ones
  - also collapse the list of contributors by default with the HTML `<details>` tag

Signed-off-by: Anton Gilgur <[email protected]>
Signed-off-by: Anton Gilgur <[email protected]>
@agilgur5 agilgur5 added area/docs Incorrect, missing, or mistakes in docs area/build Build or GithubAction/CI issues prioritized-review For members of the Sustainability Effort labels Jun 22, 2024
@@ -1100,15 +1291,22 @@
* Ruin09
* Son Bui
* Suraj Banakar(बानकर) | スラジ
* Yuan (Terry) Tang
Copy link
Member

Choose a reason for hiding this comment

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

Hmm I wonder why this shows up in addition to "Yuan Tang" below

Copy link
Contributor Author

@agilgur5 agilgur5 Jun 24, 2024

Choose a reason for hiding this comment

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

It's probably because you had a different name in your commit author previously (i.e. Author: Yuan (Terry) Tang <email>. Since the script uses pure git (no GitHub so it can't do, for instance, email -> GH user), it pulls entirely from the existing commit metadata in the repo

Copy link
Member

Choose a reason for hiding this comment

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

Is there a way to hack/merge these different ones?

Copy link
Contributor Author

@agilgur5 agilgur5 Jun 24, 2024

Choose a reason for hiding this comment

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

Not without making the script even hackier than it already is 😬
I would prefer to use GH IDs where possible, but that would require some non-trivial work to do

You'd effectively have to make a dictionary of author names to reference and choose one by some preference (I guess latest one), which means an associative array in Bash, which requires Bash 4+ (i.e. breaks on older Bash, older Macs, etc b/c Bash portability kinda sucks 😐 ). So another loop and a dictionary lookup.

I did notice this in the diff, but thought it was nbd, especially since this PR collapses the Contributors section anyway. Your own name here is though, so a bit different I get it

Copy link
Contributor Author

@agilgur5 agilgur5 Jun 24, 2024

Choose a reason for hiding this comment

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

You'd effectively have to make a dictionary of author names to reference and choose one by some preference (I guess latest one), which means an associative array in Bash

Oh wait, the author name and email are the (non-unique) identifier, so you'd actually need an associative array for old name + email to new name + email. There could be multiple matches for that and you might not be able to match if both the email and name are different -- which they very well might be in your case if you were using corporate emails.
In other words, quite hacky 🙃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO, I'd punt this until we can do GH ID lookups, but that's not perfect either with regard to historical emails/IDs. Which, to be explicit, probably wouldn't happen anytime soon given the effort to impact ratio

@juliev0 juliev0 merged commit c8073b5 into argoproj:main Jun 30, 2024
18 checks passed
@agilgur5 agilgur5 deleted the docs-changelog-full-plus-all-contributors branch June 30, 2024 19:16
agilgur5 added a commit that referenced this pull request Jul 30, 2024
Signed-off-by: Anton Gilgur <[email protected]>
(cherry picked from commit c8073b5)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/build Build or GithubAction/CI issues area/docs Incorrect, missing, or mistakes in docs prioritized-review For members of the Sustainability Effort
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants