fix: behavior with usePullRequestMetadata option #26
Conversation
| @@ -273,22 +297,15 @@ func buildReleaseCommits(ctx context.Context, ghClient *githubClient, commits [] | |||
| } | |||
|
|
|||
| if gen.UsePullRequestMetadata { | |||
| prNumber, ok := commit.PullRequestNumber() | |||
| if !ok { | |||
| continue | |||
There was a problem hiding this comment.
This is where the behavior of excluding commits for which the PR Number cannot be obtained occurs.
There was a problem hiding this comment.
got your point!
let's apply this improvement.
release.go
Outdated
| if !commit.IsMerge() { | ||
| return nil, nil | ||
| } | ||
| if pr, ok := shaPRs[commit.Hash]; ok { |
There was a problem hiding this comment.
This is not a bugfix, but an improvement.
The subject of a merge commit may not match Merge pull request #([0-9]+) from .+ regex. So use merge commit SHA instead of PR Number to identify the PR.
There was a problem hiding this comment.
Exactly, thank you for your improving.
| if pr, ok := numPRs[prNumber]; ok { | ||
| return pr, nil | ||
| } | ||
| pr, err := ghClient.getPullRequest(ctx, event.Owner, event.Repo, prNumber) |
There was a problem hiding this comment.
If the PR is not retrieved on the first fetch, the API will be called as usual using the PR Number.
| if err != nil { | ||
| return nil, err | ||
| } | ||
| c.PullRequestOwner = pr.GetUser().GetLogin() | ||
| c.ReleaseNote = extractReleaseNote(pr.GetTitle(), pr.GetBody(), gen.UseReleaseNoteBlock) | ||
| if pr != nil { |
There was a problem hiding this comment.
ReleaseNote and PR Metadata are added only when a PR is obtained. In other words, even if a PR cannot be obtained, it will be included in the Release Commit list.
release.go
Outdated
| if !commit.IsMerge() { | ||
| return nil, nil | ||
| } | ||
| if pr, ok := shaPRs[commit.Hash]; ok { |
There was a problem hiding this comment.
Exactly, thank you for your improving.
release.go
Outdated
| prNumber, ok := commit.PullRequestNumber() | ||
| if !ok { | ||
| return nil, nil | ||
| } | ||
| if pr, ok := numPRs[prNumber]; ok { | ||
| return pr, nil | ||
| } |
There was a problem hiding this comment.
Can we remove this logic by above improvement?
There was a problem hiding this comment.
Yes! deleted on 8dde46a.
| @@ -273,22 +297,15 @@ func buildReleaseCommits(ctx context.Context, ghClient *githubClient, commits [] | |||
| } | |||
|
|
|||
| if gen.UsePullRequestMetadata { | |||
| prNumber, ok := commit.PullRequestNumber() | |||
| if !ok { | |||
| continue | |||
There was a problem hiding this comment.
got your point!
let's apply this improvement.
|
Thank you for your nice job! |
| if err != nil { | ||
| return nil, err | ||
| // only error logging, ignore error | ||
| log.Printf("Failed to get pull request: %v\n", err) |
There was a problem hiding this comment.
The PR Number retrieved from the commit subject is now the user's input value, so only logging is used when an error occurs. Commits that fall into this case are assumed to be output in the conventional form without PR metadata.
|
Thank you. |
Enabling the
usePullRequestMetadataoption of thereleaseNoteGeneratoris causing a behavior that limits Release Commits to Merge Commits.However, we believe that this option should only affect Release Note generation. Therefore, we have modified the process to add metadata used for Release Note to Release Commit when this option is enabled.
In our use case we treat Release Commits as Metrics to be sent to FourKeys, so we need all the commits in the release.