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 a new flag to use as target date #72

Merged
merged 2 commits into from
Jan 24, 2022

Conversation

AvdLee
Copy link
Contributor

@AvdLee AvdLee commented Jan 19, 2022

Our previous implementation contained a bug in which the date of triggering the release would be used as a limit range for the changelog creation. This would cause incorrect changelog updates and issue messages sending out that something was released while it actually was not.

By introducing a new changelogToTag parameter, we're solving this.

@AvdLee AvdLee requested a review from Boris-Em as a code owner January 19, 2022 14:12
@AvdLee AvdLee self-assigned this Jan 19, 2022
@AvdLee AvdLee requested review from amirdew and BasThomas and removed request for Boris-Em January 19, 2022 14:12
# Conflicts:
#	Sources/GitBuddyCore/Release/ReleaseProducer.swift
@wetransferplatform
Copy link
Collaborator

Messages
📖

View more details on Bitrise

📖 GitBuddyTests: Executed 52 tests (0 failed, 0 retried, 0 skipped) in 1.074 seconds

Code Coverage Report

Name Coverage
GitBuddy 0.00% ⚠️

Generated by 🚫 Danger Swift against b0a4461

Copy link
Contributor

@BasThomas BasThomas left a comment

Choose a reason for hiding this comment

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

I'm going to leave a review as-is; I haven't looked at the Tag.swift and ReleaseProducer.swift in detail, as I'm having trouble understanding the logic.

I'm aware this is the first I've seen of this code base, so perhaps part of it is me. Happy to jump on a call to look at this with together with someone though! :)

@@ -33,6 +33,9 @@ struct ReleaseCommand: ParsableCommand {
@Option(name: .shortAndLong, help: "The last release tag to use as a base for the changelog creation. Default: previous tag.")
var lastReleaseTag: String?

@Option(name: .customLong("changelogToTag"), help: "If set, the date of this tag will be used as the limit for the changelog creation. This variable should be passed when `tagName` is set. Default: latest tag.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
@Option(name: .customLong("changelogToTag"), help: "If set, the date of this tag will be used as the limit for the changelog creation. This variable should be passed when `tagName` is set. Default: latest tag.")
@Option(name: .customLong("changelogToTag"), help: "If set, the date of this tag will be used as the limit for the changelog creation. This variable should be passed when `tagName` is set.")

As I understand this does not have a default, does it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It actually has!

let changelogToTag = try changelogToTag.map { try Tag(name: $0) } ?? Tag.latest()

The variable is required if a specific tagName is set since the latest tag is yet to be created.

There are basically two scenarios:

  • Creating a release after a tag was already created
  • Creating a release and with that creating a tag using tagName

For the first scenario, it's clear that the changelog should be created using range [previousTag - lastTag]. For the second scenario, however, we're creating a release from a certain commit or branch which we can't use as trailing boundary for the changelog creation. Therefore, it's required to help GitBuddy by informing which boundary tag to use.

Let me know if that clarifies things!

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha! I got confused about the default because the specific option variable itself did not have a default. 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yeah, I see! I guess the default value is not always provided through the command interface. Especially in this case, since it performs actual git operations at the moment of execution :)

@AvdLee AvdLee merged commit 47bda85 into feature/delete-old-tags Jan 24, 2022
@AvdLee AvdLee deleted the feature/use-correct-date branch January 24, 2022 15:18
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.

None yet

4 participants