relnotes: --list-v2 lookup PR numbers by merge commits

[wilsonehusin]

Signed-off-by: Wilson E. Husin whusin@vmware.com

What type of PR is this?

/kind feature

What this PR does / why we need it:

This introduces an experimental change behind a feature flag --list-v2 for krel release-notes.
See release notes for details.

Some benchmarks running on 1.21.0-alpha.2:

• GitHub API call to get list of commits vs local copy Git tree traversal

  # GitHub API call
  INFO finished getting list of commits: 2048.06ms
  

  # Local copy traversal
  INFO finished getting list of commits: 37.92ms
  

• Looking up PR numbers for all commits vs only from merge commits

  # Find PR number from *all* commits (triggered anti-abuse limit once)
  INFO finished gathering release notes in 1m41.465202636s
  

  # Find PR number from *merge* commits
  INFO finished gathering release notes in 21.363669925s
  

Which issue(s) this PR fixes:

None

Special notes for your reviewer:

This is initial steps towards making krel release-notes workflow more reliable and less manual intervention.

TODO:

• update & clarify in-code documentation of rewinding commit
• ensuring this mechanism works for patch releases
• NONE detection even when release-notes-none label is not present, case on point

Does this PR introduce a user-facing change?

- Downgrade logs on attempting to lookup PRs by commit to Debug as they are filling up console noisily, obstructing the progress log.
- Introducing `--list-v2` feature flag to `krel release-notes`. Behind a feature gate:
  - Looks up for the commit history from the local copy of k/k instead of GitHub API call.
  - The new approach traverses Git history by left parents, only looking for PR information from the merge commits and thus reducing the API calls to GitHub, which should decrease the amount of rate limit errors users are receiving.
  - Incidentally, this fixes a "bug" in previous implementation of including a "merged in the future" PR.

1.21 Release Notes team: Making this visible to you all -- if you're interested please give it a try and do a comparison. Same workflow, just add --list-v2 flag after you clone this.
/cc @ashnehete @melodychn @pmmalinov01 @soniasingla

Recent contributors to krel release-notes: Let me know what you all think! (also hi Chris! I haven't worked with you but have seen your work around release notes JSON, making this visible to you in case it's of interest)
/cc @puerco @saschagrunert @cartyc

For discussion, probably going to let this up for a while and give folks a chance to comment (if you're not mentioned above and have thoughts, please send them over!)
/hold

[saschagrunert]

Can we ensure that we there are always only 2 commits? This would change if we decide to cherry-pick some patches right before the release, right?

[wilsonehusin]

Ah yeah, I posted this elsewhere and I forgot to put it here -- that's correct, this approach will not work for patch releases. I think there should be some smart detection mechanism to get it, I'll put it in TODO now while explore the calculation today ish.

[saschagrunert]

That's great, thank you! 🙏

[wilsonehusin]

so I looked into this again and I think trying to support patch releases at the moment would over-complicate the code. for example, in generating patch releases of 1.20, we need to find commits from the specified tag up to 1.19.0, which points to this commit. somehow, 2-level of parents from that commit has release-1.19 on the left and master on the right, which is the opposite of the convention in 1.20.

in short, I'm suggesting is that we leave this --list-v2 experimental flag until 1.20 is the oldest release line we support before deprecating the older implementation. --list-v2 will continue to work and beneficial for release team fetching minor-release notes and patch-release notes won't even get a chance to use this until later. how does that sound?

cc @puerco @saschagrunert

[puerco]

Well, we could always plug the v2 list code into the krel release-notes ... invocations used by the release team.

Also, let's keep in mind that it's not just a question of kubernetes prereleases vs patch releases. We already have 3rd party consumers who are using the release-notes tool for other projects. How are those looking facing a potential change? Can we know?

[wilsonehusin]

so I don't expect --list-v2 implementation to change the end result. it fixed a bug which exists the old implementation, and that's the only acceptable regression IMO. anything else has to be the same, just performance improvements.

one goal that I'd like to have before removing the feature flag is to have rely on git merge-base instead of hackily rewinding commits like this, because it'd be more deterministic.

[wilsonehusin]

okay I went ahead and implemented commit resolution through merge-base and it's now working for patch releases! 🎉

[puerco]

The results your tests have been showing are great wilson.
A little note below and in a standalone comment 👇

[puerco]

I was running your fork and look what showed up in the MD notes, below ### Deprecation:

- NONE ([#96311](https://github.com/kubernetes/kubernetes/pull/96311), [@thockin](https://github.com/thockin)) [SIG API Machinery, Apps, Cloud Provider, Network, Scheduling and Testing]

At first, I did not think it was related but I checked and It is not in the current draft.

[wilsonehusin]

I was running your fork and look what showed up in the MD notes, below ### Deprecation:

- NONE ([#96311](https://github.com/kubernetes/kubernetes/pull/96311), [@thockin](https://github.com/thockin)) [SIG API Machinery, Apps, Cloud Provider, Network, Scheduling and Testing]

At first, I did not think it was related but I checked and It is not in the current draft.

🤔 Prow is not labeling as release-notes-none and I think that's the problem. I wonder if the previous implementation manually scans for NONE. I'll add this to the TODO list for now, thanks for reporting Adolfo!

[wilsonehusin]

@puerco since we're talking about release logs -- is this intentional to be in Info level? because I found it to be noisy when running release notes locally, but I wonder if this is a desired feature?

alternatively, if it is desired, would it make sense to run with Debug level for releases?

[puerco]

TBH I'm not sure when or how often this case can happen. I don't remember seeing it much scratch that, I ran a couple of tests and I see what you mean.

[puerco]

I wonder if the previous implementation manually scans for NONE
It needs a bit more investigation. The NONE detection happens here in notesForCommit() which is used by the new implementation as well:

release/pkg/notes/notes.go

Lines 681 to 687 in 139ec19

	if MatchesExcludeFilter(prBody) {
	logrus.Infof(
	"Skipping PR #%d because it contains no release note",
	pr.GetNumber(),
	)
	continue
	}

[wilsonehusin]

okay I personally feel comfortable with this for now since it's behind a feature flag and we can iterate. let me know if you think otherwise, but I'm going to cancel the hold from my end.

/hold cancel

[wilsonehusin]

ergh on second thought... I should just implement merge-base on this PR anyway since I have all the information I need.

/hold

[wilsonehusin]

merge-base has been implemented! 🎉

/hold cancel

[puerco]

Nice job @wilsonehusin !!
/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: puerco, wilsonehusin

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [puerco]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment