Implement when PreleaseLabel is empty, the PreleaseTag is generated correctly #3447

HHobeck · 2023-03-22T16:42:16Z

Fix [Bug] When PreleaseLabel is empty, the PreleaseTag is not correctly generated #2347

Close #2347

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing tests passed.

HHobeck · 2023-03-28T09:33:58Z

I'm done please review and merge to main.

arturcic · 2023-03-28T11:23:30Z

Is there any reason to have 4 new interfaces IContinuousDeliveryVersionCalculator, IContinuousDeploymentVersionCalculator, IManualDeploymentVersionCalculator, and ITrunkBasedVersionCalculator that have the same method with the same signature and to not share a IVersionCalculator?

If we take in consideration your suggestion regarding

public enum VersioningMode
{
    ManuallyDeployment, // previously ContinuousDelivery
    ContinuousDelivery, // previously ContinuousDeployment
    ContinuousDeployment, // new
    Trunkbased // previously Mainline??
}

I would rather have a IVersionCalculator that has the VersioningMode field and each implementation of this interfaces will set the appropriate VersioningMode andd then in the NextVersionCalculator I would inject all implementations and depending on the VersioningMode it will be selected the appropriate one. Thoughts?

HHobeck · 2023-03-28T14:56:59Z

Is there any reason to have 4 new interfaces IContinuousDeliveryVersionCalculator, IContinuousDeploymentVersionCalculator, IManualDeploymentVersionCalculator, and ITrunkBasedVersionCalculator that have the same method with the same signature and to not share a IVersionCalculator?

If we take in consideration your suggestion regarding
public enum VersioningMode
{
    ManuallyDeployment, // previously ContinuousDelivery
    ContinuousDelivery, // previously ContinuousDeployment
    ContinuousDeployment, // new
    Trunkbased // previously Mainline??
}
I would rather have a IVersionCalculator that has the VersioningMode field and each implementation of this interfaces will set the appropriate VersioningMode andd then in the NextVersionCalculator I would inject all implementations and depending on the VersioningMode it will be selected the appropriate one. Thoughts?

Yep good point. At this moment I have separated the behavior which not aligns with the VersioningMode enumerator. But if we are going to decide to change it so we have a one to one relation then yes I think your idea is great.

Do you have something like this in mind?

public interface IVersionCalculator
{
    VersioningMode VersioningMode { get; }

    SemanticVersion Calculate(NextVersion nextVersion);
}

public IVersionCalculator GetSpecificVersionCalculator(VersioningMode versioningMode)
{
    return _serviceProvider.GetServices<IVersionCalculator>().Single(
        element => element.VersioningMode == versioningMode
    );
}

arturcic · 2023-03-28T14:59:24Z

Is there any reason to have 4 new interfaces IContinuousDeliveryVersionCalculator, IContinuousDeploymentVersionCalculator, IManualDeploymentVersionCalculator, and ITrunkBasedVersionCalculator that have the same method with the same signature and to not share a IVersionCalculator?
If we take in consideration your suggestion regarding
public enum VersioningMode
{
    ManuallyDeployment, // previously ContinuousDelivery
    ContinuousDelivery, // previously ContinuousDeployment
    ContinuousDeployment, // new
    Trunkbased // previously Mainline??
}
I would rather have a IVersionCalculator that has the VersioningMode field and each implementation of this interfaces will set the appropriate VersioningMode andd then in the NextVersionCalculator I would inject all implementations and depending on the VersioningMode it will be selected the appropriate one. Thoughts?
Yep good point. At this moment I have separated the behavior which not aligns with the VersioningMode enumerator. But if we are going to decide to change it so we have a one to one relation then yes I think your idea is great.

Do you have something like this in mind?
public interface IVersionCalculator
{
    VersioningMode VersioningMode { get; }

    SemanticVersion Calculate(NextVersion nextVersion);
}

public IVersionCalculator GetSpecificVersionCalculator(VersioningMode versioningMode)
{
    return _serviceProvider.GetServices<IVersionCalculator>().Single(
        element => element.VersioningMode == versioningMode
    );
}

Exactly, in this case we don't need that many interfaces for the all the modes, and we have only the implementations and one interface

arturcic · 2023-03-28T15:46:32Z

Exactly, in this case we don't need that many interfaces for the all the modes, and we have only the implementations and one interface

So what do you think @HHobeck should we make those changes to the VersionMode, or put that change into another PR?

asbjornu · 2023-03-28T19:59:40Z

Exactly, in this case we don't need that many interfaces for the all the modes, and we have only the implementations and one interface

Agreed.

HHobeck · 2023-03-29T05:48:57Z

Exactly, in this case we don't need that many interfaces for the all the modes, and we have only the implementations and one interface

So what do you think @HHobeck should we make those changes to the VersionMode, or put that change into another PR?

I think it is better to do this in a separate PR. You mentioned already to do this after the beta.2 release. Which is fine with me.

HHobeck · 2023-03-29T05:53:50Z

Also we need to discuss the naming. IMO the naming of VersioningMode is missleading. Maybe a better name might be DeploymentMode. What do you think? In addition what do you like more Trunkbased or Mainline?

public enum DeploymentMode
{
    ManuallyDeployment, // previously ContinuousDelivery
    ContinuousDelivery, // previously ContinuousDeployment
    ContinuousDeployment, // new
    Trunkbased // previously Mainline??
}

arturcic · 2023-03-29T15:15:43Z

@asbjornu do you agree with this PR to be merged?

…orrectly

asbjornu

This is great stuff. The only thing I'm a bit wary about is the change from 1.0.1+46 to 1.0.1-46 unless label is set to null. Not because the change doesn't make sense, I think it's a good default, but because it's a breaking change. However, I think we should test this change out in the community and see what feedback we get.

BREAKING_CHANGES.md

asbjornu · 2023-04-06T10:26:13Z

src/GitVersion.Core/VersionCalculation/Abstractions/IContinuousDeliveryVersionCalculator.cs

@@ -0,0 +1,6 @@
+namespace GitVersion.VersionCalculation;
+
+public interface IContinuousDeliveryVersionCalculator


Didn't we agree to reduce these interfaces down to just one, as they all have the same method signature SematicVersion Calculate(NextVersion nextVersion)?

Well we discussed about that, but agreed to do the change after beta2, as it will require changing the VersioningMode

Can you please explain why merging these interfaces will require changing VersioningMode?

Please check this changes

https://github.com/GitTools/GitVersion/pull/3447/files#diff-d1bb8310a1a6168b70d713420c6ecc455c3bd3311b4735c3811fb8e249db8da4R61-R69

(NextVersionCalculator.cs / CalculateIncrementedVersion)

We currently have 3 VersionModes but we have 4 interfaces, and @HHobeck proposed to change the VersionMode to

public enum VersioningMode { ManuallyDeployment, // previously ContinuousDelivery ContinuousDelivery, // previously ContinuousDeployment ContinuousDeployment, // new Trunkbased // previously Mainline?? }

Or

public enum DeploymentMode { ManuallyDeployment, // previously ContinuousDelivery ContinuousDelivery, // previously ContinuousDeployment ContinuousDeployment, // new Trunkbased // previously Mainline?? }

Then we have 4 modes and 4 interfaces. In that case if we define a property in a single interface like

public interface IVersionCalculator { VersioningMode VersioningMode { get; } SemanticVersion Calculate(NextVersion nextVersion); }

each implementation of this interface will set the mode accordingly.

I still don't understand why we can't have one interface with 4 different implementations? I would also prefer discussing renaming and other large changes in separate issues. If we are to keep a mode for what is basically mainline, I think we can just keep the name Mainline.

Mainline is confisuing IMO because we have a IsMainline property as a branch configuration and a Mainline value on the version mode. If you say the branch needs to be mainline then it's confusing. It was just a proposal from my side because if we are touching this lines of code it's easy to do it right away. But if you don't like it nevermind.

src/GitVersion.Core/VersionCalculation/ContinuousDeliveryVersionCalculator.cs

src/GitVersion.Core/VersionCalculation/ContinuousDeploymentVersionCalculator.cs

src/GitVersion.Core/VersionCalculation/ManualDeploymentVersionCalculator.cs

src/GitVersion.Core/VersionCalculation/NonTrunkBasedVersionCalculatorBase.cs

src/GitVersion.Core/VersionCalculation/TrunkBasedVersionCalculator.cs

asbjornu

Just a couple of typos, then I think this is good.

src/GitVersion.Core/VersionCalculation/ContinuousDeliveryVersionCalculator.cs

src/GitVersion.Core/VersionCalculation/ContinuousDeploymentVersionCalculator.cs

asbjornu

Superb! 👏🏼

mergify · 2023-04-06T18:50:35Z

Thank you @HHobeck for your contribution!

HHobeck force-pushed the feature/2347_PromoteTagEvenIfNameIsEmpty branch 2 times, most recently from 5f11137 to 45ea2c0 Compare March 28, 2023 08:06

HHobeck force-pushed the feature/2347_PromoteTagEvenIfNameIsEmpty branch from 45ea2c0 to 263eb78 Compare March 29, 2023 05:49

HHobeck force-pushed the feature/2347_PromoteTagEvenIfNameIsEmpty branch from 263eb78 to 2ef39cd Compare March 29, 2023 15:13

arturcic requested review from arturcic and asbjornu March 29, 2023 15:22

arturcic approved these changes Mar 29, 2023

View reviewed changes

HHobeck force-pushed the feature/2347_PromoteTagEvenIfNameIsEmpty branch 2 times, most recently from 6d0edc6 to ae9cec8 Compare March 30, 2023 15:41

HHobeck added 2 commits April 6, 2023 08:25

Implement when PreleaseLabel is empty, the PreleaseTag is generated c…

c65b9f0

…orrectly

Documenting what change we are implementing

7f7163c

HHobeck force-pushed the feature/2347_PromoteTagEvenIfNameIsEmpty branch from 15a0970 to 7f7163c Compare April 6, 2023 06:28

asbjornu requested changes Apr 6, 2023

View reviewed changes

HHobeck added 2 commits April 6, 2023 15:05

Review remarks integrated

97f2019

Give NotImplementedException a message

3083f16

asbjornu requested changes Apr 6, 2023

View reviewed changes

Fix typos in ContinuousDelivery and DeploymentCalculator

25c17c8

asbjornu approved these changes Apr 6, 2023

View reviewed changes

arturcic merged commit 8caa9d2 into GitTools:main Apr 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement when PreleaseLabel is empty, the PreleaseTag is generated correctly #3447

Implement when PreleaseLabel is empty, the PreleaseTag is generated correctly #3447

HHobeck commented Mar 22, 2023 •

edited

Loading

HHobeck commented Mar 28, 2023

arturcic commented Mar 28, 2023

HHobeck commented Mar 28, 2023

arturcic commented Mar 28, 2023

arturcic commented Mar 28, 2023

asbjornu commented Mar 28, 2023

HHobeck commented Mar 29, 2023

HHobeck commented Mar 29, 2023

arturcic commented Mar 29, 2023

asbjornu left a comment

asbjornu Apr 6, 2023

arturcic Apr 6, 2023

asbjornu Apr 6, 2023

arturcic Apr 6, 2023 •

edited

Loading

asbjornu Apr 6, 2023

HHobeck Apr 6, 2023

asbjornu left a comment

asbjornu left a comment

mergify bot commented Apr 6, 2023

		@@ -0,0 +1,6 @@
		namespace GitVersion.VersionCalculation;

		public interface IContinuousDeliveryVersionCalculator

Implement when PreleaseLabel is empty, the PreleaseTag is generated correctly #3447

Implement when PreleaseLabel is empty, the PreleaseTag is generated correctly #3447

Conversation

HHobeck commented Mar 22, 2023 • edited Loading

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Checklist:

HHobeck commented Mar 28, 2023

arturcic commented Mar 28, 2023

HHobeck commented Mar 28, 2023

arturcic commented Mar 28, 2023

arturcic commented Mar 28, 2023

asbjornu commented Mar 28, 2023

HHobeck commented Mar 29, 2023

HHobeck commented Mar 29, 2023

arturcic commented Mar 29, 2023

asbjornu left a comment

Choose a reason for hiding this comment

asbjornu Apr 6, 2023

Choose a reason for hiding this comment

arturcic Apr 6, 2023

Choose a reason for hiding this comment

asbjornu Apr 6, 2023

Choose a reason for hiding this comment

arturcic Apr 6, 2023 • edited Loading

Choose a reason for hiding this comment

asbjornu Apr 6, 2023

Choose a reason for hiding this comment

HHobeck Apr 6, 2023

Choose a reason for hiding this comment

asbjornu left a comment

Choose a reason for hiding this comment

asbjornu left a comment

Choose a reason for hiding this comment

mergify bot commented Apr 6, 2023

HHobeck commented Mar 22, 2023 •

edited

Loading

arturcic Apr 6, 2023 •

edited

Loading