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

[Bug]? no minor version detected on pull request that contains organization name #3183

Closed
ooredm opened this issue Aug 31, 2022 · 7 comments
Closed
Labels
Milestone

Comments

@ooredm
Copy link

ooredm commented Aug 31, 2022

Context: MainlineMode.

No minor increment on merge message that contains an organization name, e.g:
"Merge pull request #1 from bananaCorp/develop"
This is the default pull request message of GitHub for an organization.

The default regex for a develop branch is, "^dev(elop)?(ment)?$", due to this the branch is not detected by the MainLineVersionCalculator, as it sees "bananaCorp/develop" as the branch. We can work around this by changing the regex to "dev(elop)?(ment)?$".

My question is if this is the intended behavior, and for what reason.

Thanks in advance.

@ooredm ooredm added the bug label Aug 31, 2022
@asbjornu
Copy link
Member

asbjornu commented Sep 1, 2022

Merge pull request #1 from bananaCorp/develop

To me, this looks like a merge from a remote into a local develop branch, instead of what it imho should have been: A fast-forward or a rebase. Merges shouldn't be necessary between branches of the same name, regardless of whether they are remote or local.

Also, the fact that you're using a develop branch tells me that you aren't really doing Mainline development. In mode: Mainline, there is only one branch that matters: The main branch. The point is to use trunk based development where no branches are visible anywhere because they are squashed into single, no-merge commits on main. If you want to use more than one branch, you should not use mode: Mainline.

The default regex for a develop branch is, "^dev(elop)?(ment)?$", due to this the branch is not detected by the MainLineVersionCalculator, as it sees "bananaCorp/develop" as the branch. We can work around this by changing the regex to "dev(elop)?(ment)?$".

If you absolutely want to do remote merges the way you do and have branches named bananaCorp/develop, you can of course remove the start of line anchor ^ from the regex. 🤷🏼 The reason the regex only matches dev, develop and development by default is to ensure consistency through enforcing a Git Flow compliant development model.

@koosvanw
Copy link

koosvanw commented Sep 2, 2022

To me, this looks like a merge from a remote into a local develop branch, instead of what it imho should have been: A fast-forward or a rebase. Merges shouldn't be necessary between branches of the same name, regardless of whether they are remote or local.

It is actually a merge which is executed by Github when merging a pullrequest, creating a merge commit. I don't know why the organization name is added in the commit message, however the branch is plainly develop. So the merge is not performed locally from a remote. In this case the merge goes from develop to master, however Github leaves out the target branch in the merge message.

If you want to use more than one branch, you should not use mode: Mainline.

What mode would you suggest in this case?

@asbjornu
Copy link
Member

asbjornu commented Sep 2, 2022

If you are closer to a Git Flow development model with develop, feature, release branches, and so on, using the default mode ContinuousDeployment or ContinuousDelivery would be better.

@HHobeck HHobeck added this to the 6.x milestone Mar 2, 2023
@HHobeck
Copy link
Contributor

HHobeck commented Mar 5, 2023

I have taken a look into the code and saw unit tests which are testing the behavior as following:

    private static readonly object?[] GitHubPullPullMergeMessages =
    {
        new object?[] { "Merge pull request #1234 from feature/one", "feature/one", null, null, 1234 },
        new object?[] { "Merge pull request #1234 in feature/one", "feature/one", null, null, 1234  },
        new object?[] { "Merge pull request #1234 in v4.0.0", "v4.0.0", null, new SemanticVersion(4), 1234  },
        new object?[] { "Merge pull request #1234 from origin/feature/one", "origin/feature/one", null, null, 1234  },
        new object?[] { "Merge pull request #1234 in feature/4.1.0/one", "feature/4.1.0/one", null, new SemanticVersion(4,1), 1234  },
        new object?[] { "Merge pull request #1234 in V://10.10.10.10", "V://10.10.10.10", null, null, 1234 },
        new object?[] { "Merge pull request #1234 from feature/one into dev", "feature/one", "dev", null, 1234  }
    };

    [TestCaseSource(nameof(GitHubPullPullMergeMessages))]
    public void ParsesGitHubPullMergeMessage(
        string message,
        string expectedMergedBranch,
        string expectedTargetBranch,
        SemanticVersion expectedVersion,
        int? expectedPullRequestNumber)
    {
        // Act
        var sut = new MergeMessage(message, this.configuration);

        // Assert
        sut.FormatName.ShouldBe("GitHubPull");
        sut.TargetBranch.ShouldBe(expectedTargetBranch);
        sut.MergedBranch.ShouldBe(expectedMergedBranch);
        sut.IsMergedPullRequest.ShouldBeTrue();
        sut.PullRequestNumber.ShouldBe(expectedPullRequestNumber);
        sut.Version.ShouldBe(expectedVersion);
    }

What me supprised is that indeed like @ooredm explained it the organization name is (always?) part of the merge message in git hub:
image

@HHobeck
Copy link
Contributor

HHobeck commented Mar 5, 2023

@ooredm To workaround this I would suggest you to change the configuration and place the following string:

merge-message-formats:
  Custom: '^Merge pull request #(?<PullRequestNumber>\d+) (from|in) (?:[^\s\/]+\/)?(?<SourceBranch>[^\s]*)(?: into (?<TargetBranch>[^\s]*))*'

@arturcic
Copy link
Member

arturcic commented Mar 6, 2023

Closed by #3424

@arturcic arturcic closed this as completed Mar 6, 2023
@arturcic arturcic modified the milestones: 6.x, 6.0.0-beta.2 Apr 6, 2023
@arturcic
Copy link
Member

arturcic commented Apr 6, 2023

🎉 This issue has been resolved in version 6.0.0-beta.2 🎉
The release is available on:

Your GitReleaseManager bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants