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

Always include end of commit range #107

Merged
merged 3 commits into from
Sep 18, 2023
Merged

Always include end of commit range #107

merged 3 commits into from
Sep 18, 2023

Conversation

wagoodman
Copy link
Contributor

@wagoodman wagoodman commented Sep 18, 2023

Currently there is erroneous logic for including or excluding the end of a commit range for a release (the end should always be included). The following diff is what this PR logically achieves:

diff --git a/chronicle/release/releasers/github/summarizer.go b/chronicle/release/releasers/github/summarizer.go
index c3f2406..f0cfedf 100644
--- a/chronicle/release/releasers/github/summarizer.go
+++ b/chronicle/release/releasers/github/summarizer.go
@@ -105,7 +105,7 @@ func (s *Summarizer) Changes(sinceRef, untilRef string) ([]change.Change, error)
        var changes []change.Change
        var err error
 
-       var includeStart, includeEnd bool
+       var includeStart bool
 
        var sinceTag *git.Tag
        sinceHash := sinceRef
@@ -135,13 +135,11 @@ func (s *Summarizer) Changes(sinceRef, untilRef string) ([]change.Change, error)
                if err != nil {
                        return nil, err
                }
-               includeEnd = false
        } else {
                untilHash, err = s.git.HeadTagOrCommit()
                if err != nil {
                        return nil, err
                }
-               includeEnd = true
        }
 
        var includeCommits []string
@@ -150,7 +148,7 @@ func (s *Summarizer) Changes(sinceRef, untilRef string) ([]change.Change, error)
                        SinceRef:     sinceHash,
                        UntilRef:     untilHash,
                        IncludeStart: includeStart,
-                       IncludeEnd:   includeEnd,
+                       IncludeEnd:   true,
                })
                if err != nil {
                        return nil, fmt.Errorf("unable to fetch commit range: %v", err)

However to properly put this change under test additional functional decomposition was necessary. Since this range selection was refactored to be more accurate it additionally considers the case where no tags were found, in which case the first commit in the repo is used.

Partially addresses #96

Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
@wagoodman wagoodman requested a review from a team September 18, 2023 12:15
@wagoodman wagoodman added the bug Something isn't working label Sep 18, 2023

pushd repos/v0.1.0-dev-repo

git config --local user.email "nope@nope.com"
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: nope.com is a real domain name that resolves. (I don't know what's there; I just checked the NS lookup). Should we instead use fake@example.com or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can make that update, but that affects more than just these new fixtures. Can we follow up with another PR that makes that adjustment universally? (I don't see a direct impact here so will leave these values as is for now)

Copy link
Contributor

Choose a reason for hiding this comment

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

Definitely ok with splitting it up. I don't think this is super urgent, since we're never going to actually email these folks, but I remember hearing that the owners of test.com or similar are very tired of getting bot spam from everyone's QA accounts, and example.com is the canonical valid domain name that no one uses.

Makefile Show resolved Hide resolved
require.NoError(t, err)

// ensure that the times are sourced similarly... but the real values don't need to be under test
gitter = mockGitter{
Copy link
Contributor

Choose a reason for hiding this comment

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

This reassignment and naming confused me a little bit. It looks like you make a real gitter, and a mock gitter, but the mock gitter has a pointer to the real gitter, and you can assign either the mock or real gitter to a variable called gitter. Maybe the comment here could be rephrased, or one of the gitter vars could get a different name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll update the comment, using the same name is intentional: any usage of the unmocked gitter would be invalid from that point forward.

type changePoint struct {
Ref string
Tag *git.Tag
Inclusive bool
Copy link
Contributor

Choose a reason for hiding this comment

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

I want to make sure I understand here. Inclusive means "the changelog should include this point"? And then below, when we generate a changelog that includes the first commit in the repo, we need to emit an inclusive change point (because the first commit is part of the first release), but subsequent releases, where we're asking for something like "commits since the tag v0.5.1", we should make an exclusive range. Is that right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's 100% correct 👍

if err != nil {
return nil, err
}
includeEnd = true
}

var includeCommits []string
if s.config.ConsiderPRMergeCommits {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand what this config does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

looks like this option never was added to the documentation, I can update that in a separate PR (here's the PR that put this feature in #40 ) . In short, when enabled the commits that comprise the release can affect which PRs get included into the release notes.

Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Copy link
Contributor

@kzantow kzantow left a comment

Choose a reason for hiding this comment

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

LGTM but I'll leave it to @willmurphyscode to approve, since this is being looked at by both of us

chronicle/release/releasers/github/summarizer_test.go Outdated Show resolved Hide resolved
internal/git/first.go Show resolved Hide resolved
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
@wagoodman wagoodman enabled auto-merge (squash) September 18, 2023 17:00
@wagoodman wagoodman merged commit cc4be36 into main Sep 18, 2023
6 checks passed
@wagoodman wagoodman deleted the range-inclusion branch September 18, 2023 17:41
@wagoodman wagoodman self-assigned this Sep 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants