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

feat(changelog): support count_tags option #599

Merged
merged 11 commits into from
Aug 13, 2024

Conversation

tisonkun
Copy link
Contributor

@tisonkun tisonkun commented Apr 9, 2024

Description

@orhun I found a patch is more expressive than issue.

This is tested locally that with count_tags = "v0.7.2" I can select commits from v0.7.0..v0.7.2 and the v0.7.1 tag doesn't break changelog.

I wonder how we can support pass this option from command line and if we can add some tests.

Motivation and Context

See GreptimeTeam/greptimedb#3650 (comment).

How Has This Been Tested?

Manually tested.

Screenshots / Logs (if applicable)

Types of Changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation (no code change)
  • Refactor (refactoring production code)
  • Other

Checklist:

  • My code follows the code style of this project.
  • I have updated the documentation accordingly.
  • I have formatted the code with rustfmt.
  • I checked the lints with clippy.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@tisonkun tisonkun requested a review from orhun as a code owner April 9, 2024 23:10
Copy link

welcome bot commented Apr 9, 2024

Thanks for opening this pull request! Please check out our contributing guidelines! ⛰️

@tisonkun
Copy link
Contributor Author

tisonkun commented Apr 9, 2024

Before:
$ GITHUB_TOKEN=*** git cliff v0.7.0..v0.7.2

v0.7.2

Release date: April 08, 2024

Breaking changes

  • refactor!: Renames the new memtable to PartitionTreeMemtable by @evenyag in #3547
  • fix!: columns table in information_schema misses some columns by @killme2008 in #3639

🚀 Features

🐛 Bug Fixes

  • fix: clone data instead of moving it - homemade future is dangerous by @waynexia in #3542
  • fix: performance degradation caused by config change by @v0y4g3r in #3556
  • fix(flow): Arrange get range with batch unaligned by @discord9 in #3552
  • fix: set http response chartset to utf-8 when using table format by @xxxuuu in #3571
  • fix: update pk_cache in compat reader by @waynexia in #3576
  • fix: incorrect version info in by @waynexia in #3586
  • fix: canonicalize catalog and schema names by @killme2008 in #3600
  • fix: adjust status code to http error code map by @waynexia in #3601
  • fix: run purge jobs in another scheduler by @evenyag in #3621
  • fix: mistakely removes compaction inputs on failure by @v0y4g3r in #3635
  • fix: move object store read/write timer into inner by @dimbtp in #3627
  • fix: construct correct pk list with pre-existing pk by @waynexia in #3614

🚜 Refactor

📚 Documentation

🧪 Testing

  • test: add a parameter type mismatch test case to sql integration test by @xxxuuu in #3568
  • test: add tests for drop databases by @WenyXu in #3594
  • test: add more integration test for kafka wal by @niebayes in #3190
  • test(sqlness): release databases after tests by @WenyXu in #3648

⚙️ Miscellaneous Tasks

Build

New Contributors

All Contributors

We would like to thank the following contributors from the GreptimeDB community:

@CookiePieWw, @J0HN50N133, @MichaelScofield, @Taylor-lagrange, @WenyXu, @YCCDSZXH, @ZonaHex, @dimbtp, @discord9, @etolbakov, @evenyag, @fengjiachun, @killme2008, @niebayes, @shuiyisong, @sunng87, @tisonkun, @v0y4g3r, @waynexia, @xxxuuu, @zhongzc

v0.7.1

Release date: March 14, 2024

Breaking changes

  • fix!: remove error message from http header to avoid panic by @sunng87 in #3506

🚀 Features

🐛 Bug Fixes

🚜 Refactor

  • refactor: separate the quote char and value by @WenyXu in #3455
  • refactor: introduce new Output with OutputMeta by @shuiyisong in #3466
  • refactor: make http api returns non-200 status code by @crwen in #3473
  • refactor: validate constraints eagerly by @tisonkun in #3472

📚 Documentation

⚡ Performance

  • perf: Reduce decode overhead during pruning keys in the memtable by @evenyag in #3415
  • perf: more benchmarks for memtables by @evenyag in #3491

🧪 Testing

⚙️ Miscellaneous Tasks

Build

New Contributors

All Contributors

We would like to thank the following contributors from the GreptimeDB community:

@MichaelScofield, @Taylor-lagrange, @WenyXu, @ZonaHex, @crwen, @discord9, @etolbakov, @evenyag, @fengjiachun, @gcmutator, @shuiyisong, @sunng87, @tisonkun, @v0y4g3r, @waynexia, @zhongzc

After:
$ GITHUB_TOKEN=*** /Users/tison/Brittani/git-cliff/target/debug/git-cliff v0.7.0..v0.7.2

v0.7.2

Release date: April 08, 2024

Breaking changes

  • fix!: remove error message from http header to avoid panic by @sunng87 in #3506
  • refactor!: Renames the new memtable to PartitionTreeMemtable by @evenyag in #3547
  • fix!: columns table in information_schema misses some columns by @killme2008 in #3639

🚀 Features

🐛 Bug Fixes

🚜 Refactor

📚 Documentation

⚡ Performance

  • perf: Reduce decode overhead during pruning keys in the memtable by @evenyag in #3415
  • perf: more benchmarks for memtables by @evenyag in #3491

🧪 Testing

  • test: add fuzz test for create table by @WenyXu in #3441
  • test: add a parameter type mismatch test case to sql integration test by @xxxuuu in #3568
  • test: add tests for drop databases by @WenyXu in #3594
  • test: add more integration test for kafka wal by @niebayes in #3190
  • test(sqlness): release databases after tests by @WenyXu in #3648

⚙️ Miscellaneous Tasks

Build

New Contributors

All Contributors

We would like to thank the following contributors from the GreptimeDB community:

@CookiePieWw, @J0HN50N133, @MichaelScofield, @Taylor-lagrange, @WenyXu, @YCCDSZXH, @ZonaHex, @crwen, @dimbtp, @discord9, @etolbakov, @evenyag, @fengjiachun, @gcmutator, @killme2008, @niebayes, @shuiyisong, @sunng87, @tisonkun, @v0y4g3r, @waynexia, @xxxuuu, @zhongzc

@codecov-commenter
Copy link

codecov-commenter commented Apr 9, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 0% with 9 lines in your changes missing coverage. Please review.

Project coverage is 39.54%. Comparing base (388b007) to head (9badaa0).

Files Patch % Lines
git-cliff/src/lib.rs 0.00% 9 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #599      +/-   ##
==========================================
- Coverage   39.80%   39.54%   -0.26%     
==========================================
  Files          20       20              
  Lines        1598     1606       +8     
==========================================
- Hits          636      635       -1     
- Misses        962      971       +9     
Flag Coverage Δ
unit-tests 39.54% <0.00%> (-0.26%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: tison <wander4096@gmail.com>
@tisonkun tisonkun force-pushed the support-count-tags branch from 47d4ef3 to 63cffa7 Compare April 9, 2024 23:16
@orhun
Copy link
Owner

orhun commented Apr 13, 2024

Hey, thanks for the PR! 🐻

This option is pretty confusing to me right now, due to the fact that we have a couple of *_tags options in the config and each time I need to remember what they were doing 😅

Can you remind me (with an example) in which use case this new option will come in handy? We should also mention this in the documentation as well.

I'm happy to move on with this PR, or have a simpler solution based on what your use case is.

@tisonkun
Copy link
Contributor Author

@orhun Please take a look at #599 (comment). This is an example.

Mainly, when the range selected contains multiple tags, instead of ignore_tags, we use an inverted condition to choose which retains, without skipping any commits.

@tisonkun
Copy link
Contributor Author

We should also mention this in the documentation as well.

Sure. Where is the doc file?

Also I'd like to know if it's automatically add a cli arg or I should modify other source code as well.

@tisonkun
Copy link
Contributor Author

@orhun as a reminder. Any further thoughts here?

@orhun
Copy link
Owner

orhun commented Apr 30, 2024

Sorry for the delay! I just took a look at this again and I think I understood your use case. But just to verify: do you want to include a tag in the changelog even if it is skipped or in the given range?

This is tested locally that with count_tags = "v0.7.2" I can select commits from v0.7.0..v0.7.2 and the v0.7.1 tag doesn't break changelog.

What do you mean by "breaking" the changelog here? Do you mean splitting it into two?

It sounds like a very edge-case use-case though, so I'm not sure how to make this option intuitive for regular users. First step would be renaming count_tags to include_tags but I then we will have skip_tags, ignore_tags and include_tags and things might get even more complicated in the future. I would rather find a solution that does not require adding a new config option, if possible.

Btw I didn't fully get the example you shared, since there were many commits and it makes it hard to read. Can you maybe share it again with minimal git history, such as git-cliff-readme-example?

I just want to make sure everything is clear before moving on with this 🐻

@tisonkun
Copy link
Contributor Author

tisonkun commented May 3, 2024

include a tag in the changelog even if it is skipped or in the given range

I think the most understandable description is:

It's hard to write reverse matches with ignore_tags, count_tags is the reverse version.

What do you mean by "breaking" the changelog here? Do you mean splitting it into two?

Yes.

@tisonkun
Copy link
Contributor Author

tisonkun commented May 3, 2024

This is the simplified example:

Before:
$ GITHUB_TOKEN=*** git cliff v0.7.0..v0.7.2

v0.7.2

Release date: April 08, 2024

Breaking changes

  • refactor!: Renames the new memtable to PartitionTreeMemtable by @evenyag in #3547
  • fix!: columns table in information_schema misses some columns by @killme2008 in #3639

v0.7.1

Release date: March 14, 2024

Breaking changes

  • fix!: remove error message from http header to avoid panic by @sunng87 in #3506
After:
$ GITHUB_TOKEN=*** /Users/tison/Brittani/git-cliff/target/debug/git-cliff v0.7.0..v0.7.2

v0.7.2

Release date: April 08, 2024

Breaking changes

  • fix!: remove error message from http header to avoid panic by @sunng87 in #3506
  • refactor!: Renames the new memtable to PartitionTreeMemtable by @evenyag in #3547
  • fix!: columns table in information_schema misses some columns by @killme2008 in #3639

@tisonkun
Copy link
Contributor Author

@orhun do you still need extra information?

@orhun
Copy link
Owner

orhun commented Jun 1, 2024

Yes, it is more clear now. If this new option is going act as a reverse match to ignore_tags, I would prefer to have that functionality as part of the ignore_tags as well. Like I said before, I think having multiple *_tags options is confusing and will be more confusing if we keep adding new ones. (hint #559)

How about adding a special case to ignore_tags as follows:

[git]
ignore_tags = "!v0.7.2"

Feels a bit hacky but I think it's more clean. What do you think?

@tisonkun
Copy link
Contributor Author

tisonkun commented Jun 2, 2024

@orhun Yeah. I can understand this alternative and it can resolve my issue. Could you write a patch in this way? I don't know how to tune the revert rules in details.

A downside can be, when you'd actually like to combine include and exclude rules, with override and "OR" logic, it may run into a super complex scenarios if we patching on an option instead of make each action a single option. But we may not reach there.

@orhun
Copy link
Owner

orhun commented Jun 2, 2024

Sure, I'm happy to work on it! I need to come up with an example case for testing this feature though. Can you help me with that?

More specifically, I would like to add a test fixture for making sure that we have the correct functionality. For that, I need a list of commits as follows, example configuration and expected output.

git commit --allow-empty -m "Initial commit"
git commit --allow-empty -m "feat: add feature 1"
git commit --allow-empty -m "fix: fix feature 1"
git tag v0.1.0
git commit --allow-empty -m "feat(gui): add feature 2"
git commit --allow-empty -m "fix(gui): fix feature 2"
git tag v0.2.0
git commit --allow-empty -m "test: add tests"

You can check out this commit to see how to add new test fixtures.

@tisonkun
Copy link
Contributor Author

tisonkun commented Jun 9, 2024

@orhun fixture cases pushed. You can view the expected result and do refactor as in your mind :D

Signed-off-by: tison <wander4096@gmail.com>
@tisonkun tisonkun force-pushed the support-count-tags branch from beef585 to f219ebd Compare June 9, 2024 02:03
Signed-off-by: tison <wander4096@gmail.com>
@tisonkun
Copy link
Contributor Author

@orhun let's give this patch another time slice?

@orhun
Copy link
Owner

orhun commented Jun 21, 2024

Sorry for the delay 😓

But this indicates a bit that condition combination can be complex and if we merge the positive and negative condition in one option, it may not be less complex.

Yes, you are right. My initial assumption was ignore_tags will be matching a single expression, but taking a closer look at the fixture test you added (thanks a lot btw) I see that we need to have both ignore_tags and count_tags options to get the desired result. At this point I'm also not sure how to implement the negated expressions - I think it will overcomplicate things for no reason.

Btw I also realized another thing, you can also get the same result with the following config (w/o count_tags):

[git]
# regex for skipping tags
skip_tags = "v0.1.0-beta.1"
# regex for ignoring tags
ignore_tags = "v.*-beta.*|v0.1.0"
# Changelog

All notable changes to this project will be documented in this file.

## [0.2.0] - 2021-01-22

### Feat

- Add feature 1
- Fix feature 1
- Add feature 2
- Add feature 3

<!-- generated by git-cliff -->

Have you considered using it like this?

@tisonkun
Copy link
Contributor Author

Btw I also realized another thing, you can also get the same result with the following config

This is a good workaround, unless we have too many tags to exclude or the straightforward expression of the intention is "include".

I can live with current options, but it's not quite straightforward :O

@orhun
Copy link
Owner

orhun commented Jun 25, 2024

What do you say if we document the usage of this option better in this PR and merge it along with the fixtures? I'm not sure having count_tags option is absolutely necessary right now.

I can always come back to it if more people want this 🐻 (I resisted to adding GitHub integration for a long time until I saw people really needed it. It happens ⚔️ )

@tisonkun
Copy link
Contributor Author

tisonkun commented Jul 1, 2024

@orhun Your suggestion sounds good. Let me try to update the document for count_tags and make this a mergable patch.

@tisonkun
Copy link
Contributor Author

I come back that GreptimeTeam/greptimedb#4379 shows invert the logics and trying to exclude tags is hard. It's quite straightforward that I'd "include" only one tag.

Let me update this patch a little bit. And @orhun I'd like to know how to specify these tags from command line. Because my use case would be:

GITHUB_TOKEN=xxx git cliff v0.8.2..v0.9.0 --count-tags "v0.9.0"

The tag varies each time so it's even unnecessary to check in the cliff.toml file.

@tisonkun
Copy link
Contributor Author

I found src/args.rs now. Let me take a closer look.

tisonkun added 2 commits July 16, 2024 11:22
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
@tisonkun
Copy link
Contributor Author

@orhun Updated. Please take a look.

Signed-off-by: tison <wander4096@gmail.com>
@tisonkun
Copy link
Contributor Author

I don't know why "Check NodeJS tarball" failed. Doesn't seem like related to this patch.

git-cliff/src/lib.rs Outdated Show resolved Hide resolved
@orhun
Copy link
Owner

orhun commented Jul 27, 2024

Yup, CI failure is not related. I will come back to this PR soon, sorry for the wait 🐒

Signed-off-by: tison <wander4096@gmail.com>
@tisonkun
Copy link
Contributor Author

tisonkun commented Aug 9, 2024

@orhun master conflict. I merge and resolve that. Hopefully we can get a review in time to avoid further conflict.

Signed-off-by: tison <wander4096@gmail.com>
@tisonkun
Copy link
Contributor Author

tisonkun commented Aug 9, 2024

https://github.com/orhun/git-cliff/actions/runs/10313078397/job/28549294078?pr=599 Seems unrelated? Or some integration tests setup issue I'm unfamiliar with.

@tisonkun
Copy link
Contributor Author

ping @orhun any estimate this patch gets another review?

@orhun
Copy link
Owner

orhun commented Aug 12, 2024

All looks good, will get to merging this today probably 👍🏼

@orhun
Copy link
Owner

orhun commented Aug 12, 2024

Only thing left is I would like to mention #599 (comment) (alternative) in the docs very shortly. I can do it before merging or feel free to do it if you have time :)

@tisonkun
Copy link
Contributor Author

I can do it before merging

Yeah. I don't get it very clear. Please go ahead and I'll learn what you mean :D

Copy link
Owner

@orhun orhun left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the contribution (and your patience!)

@orhun orhun changed the title feat: support count_tags option feat(changelog): support count_tags option Aug 13, 2024
@orhun orhun merged commit b8045e9 into orhun:main Aug 13, 2024
58 of 59 checks passed
Copy link

welcome bot commented Aug 13, 2024

Congrats on merging your first pull request! ⛰️

@tisonkun tisonkun deleted the support-count-tags branch August 13, 2024 09:29
@tisonkun
Copy link
Contributor Author

@orhun Thanks for your review and knowledge on the software!

I wonder if there is an estimate of a new release including this feature or where I can subscribe for the new release.

@orhun
Copy link
Owner

orhun commented Aug 13, 2024

Ah, I can probably get to it next week (- there is a conference coming up so I will be travelling).

You can watch the repo for new releases or join the Discord for announcements :)

@tisonkun
Copy link
Contributor Author

Cool! Thank you.

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