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

Add options to specify header level and version prefix #336

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

Conversation

RaumZeit
Copy link

@RaumZeit RaumZeit commented Apr 2, 2024

Hi,

in our project, the CHANGELOG.md file deviates a little from what is expected by the current github actions script. In particular, the header level of the version specific changelog entries is H3 instead of H2. In addition, the header not only states the version number (x.y.z), but contains a prefix string, such that each line looks like:

[Version x.y.z]

instead of just

[x.y.z]

To account for these deviations, I've modified your script to allow for additional, optional arguments:

  • specifying the expected header level via the header_level input
  • adding (expecting) a prefix to the version string via the version_prefix input

I thought, it might be useful for others, so I want to share. The additional inputs are documented and examples are given in README.md as well.

It would be nice, if it would be integrated into your mainstream release.

Cheers,
RaumZeit

@RaumZeit RaumZeit requested a review from ffurrer2 as a code owner April 2, 2024 13:08
src/index.js Fixed Show resolved Hide resolved
@RaumZeit RaumZeit force-pushed the feature/viennarna_changelog branch from 86eccbb to 5a7f430 Compare May 14, 2024 09:25
@ffurrer2 ffurrer2 force-pushed the feature/viennarna_changelog branch 2 times, most recently from 2e8ff29 to 8ffac52 Compare May 14, 2024 22:11
@ffurrer2
Copy link
Owner

@RaumZeit Could you please add your changes to the changelog file? Other than that, LGTM.

@RaumZeit RaumZeit force-pushed the feature/viennarna_changelog branch 4 times, most recently from 1f77a7f to 1c228ea Compare May 15, 2024 10:57
@RaumZeit
Copy link
Author

Need some more time to fix the No newline at end of file error in ci.yml output for test/testdata/CHANGELOG_6.md (see https://github.com/RaumZeit/extract-release-notes/actions/runs/9094436202/job/24995513576#step:28:27) ...
Once this is done, I think we are good to go...

@RaumZeit RaumZeit force-pushed the feature/viennarna_changelog branch 2 times, most recently from 1c228ea to b0de1ff Compare May 15, 2024 14:19
@RaumZeit
Copy link
Author

Need some more time to fix the No newline at end of file error in ci.yml output for test/testdata/CHANGELOG_6.md (see https://github.com/RaumZeit/extract-release-notes/actions/runs/9094436202/job/24995513576#step:28:27) ... Once this is done, I think we are good to go...

Seems good now!
Apparently, the release note that is extracted is always trimmed at the beginning and end. So the expected results file must not contain an end-of-file newline if we use diff for checking against the actual result.

@ajalt
Copy link

ajalt commented May 17, 2024

In my opinion, version_prefix is not generic enough. It doesn't support e.g. text after the version number. In my case that caused #339, I don't surround my version numbers with brackets. I'd suggest a more flexible solution like the ability to specify the regex pattern that matches header lines in your file.

For example, you could get your prefix and h3 behavior by configuring this:

uses: ffurrer2/extract-release-notes@v2
header_regex: '### \[Version \d+\.\d+\.\d+\]'

And I could match my format with:

uses: ffurrer2/extract-release-notes@v2
header_regex: '## \d+\.\d+\.\d+'

This would also make header_level unnecessary.

@RaumZeit
Copy link
Author

RaumZeit commented May 21, 2024

In my opinion, version_prefix is not generic enough. It doesn't support e.g. text after the version number. In my case that caused #339, I don't surround my version numbers with brackets. I'd suggest a more flexible solution like the ability to specify the regex pattern that matches header lines in your file.

For example, you could get your prefix and h3 behavior by configuring this:

uses: ffurrer2/extract-release-notes@v2
header_regex: '### \[Version \d+\.\d+\.\d+\]'

And I could match my format with:

uses: ffurrer2/extract-release-notes@v2
header_regex: '## \d+\.\d+\.\d+'

This would also make header_level unnecessary.

Hey,
in principal, I'd support this generalization. However, including the header level in the regex is not as simple as you might think. It would be ambiguous where a release note actually ends, since either a 'level-up' or next header may end the latest release note. So, always assuming that the CHANGELOG is an actual markdown file with headers separating the content to extract seems fair to me! Otherwise, one would require extracting the version header level from the regex string automatically. On another note, handing over the entire regex to the user input is dangerous in terms of code-injection. That's why the version_prefix is always escaped before is use in the internal regex string...

On the other hand, for situations where the version is not surrounded by brackets, I'd rather relax the internal regex. Then, keeping just the prefix would suffice, since any suffixes are ignored anyway...

The required changes have just been pushed to my branch here...

@ajalt
Copy link

ajalt commented May 21, 2024

If you need to parse header level separately, you can still support a custom regex if it just matches the header title (i.e. what your version_match_regex does now).

uses: ffurrer2/extract-release-notes@v2
version_regex: '\[Version \d+\.\d+\.\d+\]'
unreleased_regex: '\[Unreleased\]
header_level: 3

I'm not sure what code injections dangers you're referring to; people are running this on their own code, and it's just a regex so the worst case scenario is that it matches the wrong line. Escaping the prefix makes sense since it's a plaintext string, but a custom regex wouldn't need any escaping.

I'm glad you relaxed the bracket requirements since it fixes my case, but if someone has a format like e.g. ## Version [1.2.3], there would be no way to support that with the current design, so giving users a more flexible option for configuring their workflow would be nice.

@ffurrer2 ffurrer2 force-pushed the feature/viennarna_changelog branch 2 times, most recently from 6423870 to 7d8d14a Compare June 16, 2024 19:55
@ffurrer2 ffurrer2 force-pushed the feature/viennarna_changelog branch from 7d8d14a to dbe4161 Compare July 4, 2024 21:03
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