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 --ancestors-only and --filter flags + some much needed maintenance #156

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

Conversation

chelnak
Copy link
Owner

@chelnak chelnak commented May 7, 2024

This change adds two new flags to the new command.

--ancestors-only

When this flag is passed, the builder will filter out any tags that are not an ancestor of the current branch. This is a possible fix for #145 .

--filter

When this flag is passed, the builder will filter out any tags that do not match the given RE2 regexp.

Additionally this PR contains some needed fixes and 💅

  • Cleaning up interface usage so that they are defined where they are consumed
  • Updated the logger so that it no longer needs to be passed as a dependency to the builder. It can be imported and called like log.Infof(....).
  • Updated go-gh to v2 and use the default graphql implementation
  • Some test cleaning, along with dependency updates

@chelnak chelnak added the enhancement New feature or request label May 7, 2024
@chelnak chelnak self-assigned this May 7, 2024
@chelnak
Copy link
Owner Author

chelnak commented May 7, 2024

Hey @h0tw1r3

It's a pretty meaty PR but it would be awesome if you could pull it down and take it for a spin.

Your comment in #145 about the tags api response is right! The PR actually switches to using Git to get the tags. Then uses merge-base to determine if the commit is an ancestor of the current branch or not.

I think the original decision to go with the api for tags was that when run in CI, the results were not consistent with some local tests. IIRC, I mainly tested on some big module repos with big complicated histories (puppetlabs-chocolatey & puppetlabs-docker), so i'd be interested to see how this performs.

With regards to the suggestion you made in #145, if we can get all of the information needed with one git command.. that would be pretty sweet.

@chelnak
Copy link
Owner Author

chelnak commented May 7, 2024

Looks like i've missed something in the tests.. will investigate and update tomorrow

@h0tw1r3
Copy link
Contributor

h0tw1r3 commented May 7, 2024

Hey @h0tw1r3

It's a pretty meaty PR but it would be awesome if you could pull it down and take it for a spin.

Yeah, lol. I'd say. Code review won't be a thing, but I'll test it on a few repos.

Your comment in #145 about the tags api response is right! The PR actually switches to using Git to get the tags. Then uses merge-base to determine if the commit is an ancestor of the current branch or not.

I think the original decision to go with the api for tags was that when run in CI, the results were not consistent with some local tests. IIRC, I mainly tested on some big module repos with big complicated histories (puppetlabs-chocolatey & puppetlabs-docker), so i'd be interested to see how this performs.

The inconsistency usually comes from how the repo was cloned, reach-ability (orphans with tags, oh my), and mixing tag types. The GH API consistently returns everything.

With regards to the suggestion you made in #145, if we can get all of the information needed with one git command.. that would be pretty sweet.

@chelnak
Copy link
Owner Author

chelnak commented May 7, 2024

🤣 I will probably split this up to be honest. It would make better sense as smaller PRs.

In the meantime, the concept is here and it's good for testing.

@h0tw1r3
Copy link
Contributor

h0tw1r3 commented May 8, 2024

Filter doesn't work for me, or I'm using it wrong. --filter="[0-7]*" on the puppet repo filters nothing.

Calling merge-base for every tag is very inefficient, almost twice as long to execute on a large repo with hundreds of tags. Did a test with it removed, by optionally adding --merged=HEAD to Git.Tags() exec. Twice as fast without exec bomb.

Before

time ~/dev/gh-changelog/gh-changelog new --ancestors-only
✓ Open CHANGELOG.md or run 'gh changelog show' to view your changelog.

real    1m55.764s
user    0m41.955s
sys     0m2.730s

After

time ~/dev/gh-changelog/gh-changelog new --ancestors-only
✓ Open CHANGELOG.md or run 'gh changelog show' to view your changelog.

real    1m15.187s
user    0m0.576s
sys     0m0.156s

Confirmed CHANGELOG.md is the same before and after.

gh-changelog-145...h0tw1r3:gh-changelog:gh-changelog-145-faster

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants