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

Fix issue 3074 by ignoring commits in several places #3173

Closed

Conversation

rominator1983
Copy link
Contributor

@rominator1983 rominator1983 commented Aug 24, 2022

Description

Previously commits where only ignored (by SHA or date) in one place when getting the base versions.
The info if a commit is ignored or not is now produced right when getting the commit log and can then be used where needed.
This info is now also evaluated when getting the increment of a single commit out of it's commit message.

Up to now there could have been commits leading to a version raise (through their commit message) even though they were ignored by date or explicitly by SHA. This has been ammended.

Related Issue

Fixes #3074.

Motivation and Context

Bit-bucket in its default configuration adds the latest 20 commit message of a source branch to the commit of a PR merge. This multiplies the occurrences of "+semver:major" commit messages in the log which can't possibly be undone if you just see this some weeks later and there are several people on the project.
When doing PRs to merge from develop to a feature branch this can result in unwanted version raises.

How Has This Been Tested?

I implemented/adapted some integration tests that were red to begin with and got green when I implemented my changes. When deactivating my changes several tests got red too.
Where I did refactorings I checked if their were tests for the previous code by uncommenting parts of it (See changes in BaseVersionCalculator).

Screenshots (if appropriate):

Checklist:

  • My code follows the code style of this project. (I was using VS which should enforce .editorconfig)

... My change requires a change to the documentation. (The documentation already stated that you could ignore single commits by SHA which was not true for all cases. So the code now conforms better to the documented behavior)

... I have updated the documentation accordingly. (No.)

@rominator1983
Copy link
Contributor Author

it seems to me the Mac tests have some instability since there is an error because of a corupt git repository or something.

@rominator1983
Copy link
Contributor Author

rominator1983 commented Aug 24, 2022

this one seems to fail in https://github.com/GitTools/GitVersion/runs/8004381283?check_suite_focus=true on macos latest but nowhere else. Is this flaky? Would a restart (don't know if I could do that) help?

Failed CacheKeyForWorktree [2 m 2 s]
    Error Message:
     System.Exception : There was an unknown problem with the Git repository you provided
    ----> LibGit2Sharp.LibGit2SharpException : SecureTransport error: connection closed via error
    Stack Trace:
       at GitVersion.GitRepository.Clone(String sourceUrl, String workdirPath, AuthenticationInfo auth) in /Users/runner/work/GitVersion/GitVersion/src/GitVersion.LibGit2Sharp/Git/GitRepository.cs:line 139
     at GitVersion.IgnoredFilteringGitRepositoryDecorator.Clone(String sourceUrl, String workdirPath, AuthenticationInfo auth) in /Users/runner/work/GitVersion/GitVersion/src/GitVersion.LibGit2Sharp/Git/IgnoredFilteringGitRepositoryDecorator.cs:line 46
     at GitVersion.GitPreparer.<>c__DisplayClass16_0.<CloneRepository>b__0() in /Users/runner/work/GitVersion/GitVersion/src/GitVersion.Core/Core/GitPreparer.cs:line 142
     at GitVersion.Helpers.RetryAction`1.<>c__DisplayClass1_0.<Execute>b__0() in /Users/runner/work/GitVersion/GitVersion/src/GitVersion.Core/Helpers/RetryAction.cs:line 16
     at Polly.Policy`1.<>c__DisplayClass11_0.<Execute>b__0(Context _, CancellationToken _)
     at Polly.Retry.RetryEngine.Implementation[TResult](Func`3 action, Context context, CancellationToken cancellationToken, ExceptionPredicates shouldRetryExceptionPredicates, ResultPredicates`1 shouldRetryResultPredicates, Action`4 onRetry, Int32 permittedRetryCount, IEnumerable`1 sleepDurationsEnumerable, Func`4 sleepDurationProvider)
     at Polly.Retry.RetryPolicy`1.Implementation(Func`3 action, Context context, CancellationToken cancellationToken)
     at Polly.Policy`1.Execute(Func`3 action, Context context, CancellationToken cancellationToken)
     at Polly.Policy`1.Execute(Func`1 action)
     at GitVersion.Helpers.RetryAction`2.Execute(Func`1 operation) in /Users/runner/work/GitVersion/GitVersion/src/GitVersion.Core/Helpers/RetryAction.cs:line 35
     at GitVersion.Helpers.RetryAction`1.Execute(Action operation) in /Users/runner/work/GitVersion/GitVersion/src/GitVersion.Core/Helpers/RetryAction.cs:line 14
     at GitVersion.GitPreparer.CloneRepository(String repositoryUrl, String gitDirectory, AuthenticationInfo auth) in /Users/runner/work/GitVersion/GitVersion/src/GitVersion.Core/Core/GitPreparer.cs:line 142
     at GitVersion.GitPreparer.CreateDynamicRepository(String targetBranch) in /Users/runner/work/GitVersion/GitVersion/src/GitVersion.Core/Core/GitPreparer.cs:line 120
     at GitVersion.GitPreparer.PrepareInternal(GitVersionOptions gitVersionOptions) in /Users/runner/work/GitVersion/GitVersion/src/GitVersion.Core/Core/GitPreparer.cs:line 57
     at GitVersion.GitPreparer.Prepare() in /Users/runner/work/GitVersion/GitVersion/src/GitVersion.Core/Core/GitPreparer.cs:line 48
     at GitVersion.Core.Tests.GitVersionExecutorTests.CacheKeyForWorktree() in /Users/runner/work/GitVersion/GitVersion/src/GitVersion.Core.Tests/Core/GitVersionExecutorTests.cs:line 98
  --LibGit2SharpException
     at LibGit2Sharp.Core.Ensure.HandleError(Int32 result) in /_/LibGit2Sharp/Core/Ensure.cs:line 136
     at LibGit2Sharp.Core.Proxy.git_clone(String url, String workdir, GitCloneOptions& opts) in /_/LibGit2Sharp/Core/Proxy.cs:line 280
     at LibGit2Sharp.Repository.Clone(String sourceUrl, String workdirPath, CloneOptions options) in /_/LibGit2Sharp/Repository.cs:line 793
     at GitVersion.GitRepository.Clone(String sourceUrl, String workdirPath, AuthenticationInfo auth) in /Users/runner/work/GitVersion/GitVersion/src/GitVersion.LibGit2Sharp/Git/GitRepository.cs:line 114
    Standard Output Messages:
   Initialized empty Git repository in /private/var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/TestRepositories/9f4b4e91-97f8-4645-9fc3-26f4037c4639/.git/
   Created git repository at '/var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/TestRepositories/9f4b4e91-97f8-4645-9fc3-26f4037c4639'
   **Visualisation of test:**
   
   @startuml
   @enduml
   

@rominator1983
Copy link
Contributor Author

Ok. the test CacheKeyForWorktree was breaking in 3 consecutive runs on macos latest and is green anywhere else.
Curiously enough similiar code is also run in other unit tests and it's always this test that breaks.
The code that breaks is already in the RetryAction.
I added a [Retry(10)] to the test in order to find out more about it.

Did anyone already look into that?

@arturcic
Copy link
Member

I noticed that as well, the macos with netcoreapp3.1 tests. For now re-running the failed job helps, but we will sunset .netcoreapp3.1 soon anyway, so we might disable the tests for this specific configuration

@rominator1983
Copy link
Contributor Author

I noticed that as well, the macos with netcoreapp3.1 tests. For now re-running the failed job helps, but we will sunset .netcoreapp3.1 soon anyway, so we might disable the tests for this specific configuration

Thx for the info. Do you think I should remove the Retry-Attribute that I just added to the test? Since the code itself is also doing a retry I figured it would do no real harm to anyone.

@arturcic
Copy link
Member

arturcic commented Aug 25, 2022

@rominator1983 please rebase your PR on main, I have disabled the tests for macos netcoreapp3.1

@arturcic
Copy link
Member

please rebase you PR on main, I have disabled the tests for macos netcoreapp3.1

@rominator1983 rominator1983 force-pushed the fix/IgnoreAllCommits_Issue3074 branch from 222c92b to f28ac82 Compare August 25, 2022 15:29
@rominator1983
Copy link
Contributor Author

Thanks @arturcic !
I just pushed my rebase and of course removed the Retry-attribute on the test.

@rominator1983
Copy link
Contributor Author

When do you think can this be made available?

@asbjornu
Copy link
Member

I'm a bit uneasy about all the removed and modified tests. I would also like @ibvhefe to take this PR for a spin to see if it solves their issue.

@ibvhefe
Copy link

ibvhefe commented Sep 28, 2022

Sorry. I need this for a private project and I barely find the time.
As I have no push permissions, here is a unit test that would fail. I am unsure about it. Does the test make sense?

Add this to IgnoreBeforeScenarios.cs

    [Test]
    public void CommitsWithSemverCommentsAreIgnored()
    {
        using var fixture = new EmptyRepositoryFixture();
        fixture.Repository.MakeATaggedCommit("1.0.0");
        fixture.Repository.CreateBranch("feature/foo");
        fixture.Checkout("feature/foo");
        fixture.Repository.MakeACommit("2");
        fixture.MakeACommit("3 +semver:minor");
        fixture.Checkout(MainBranch);
        fixture.Repository.MergeNoFF("feature/foo");
        var commit = fixture.Repository.MakeACommit("4 +semver:major");
        var config = new ConfigurationBuilder()
            .Add(new Config
            {
                VersioningMode = GitVersion.VersionCalculation.VersioningMode.Mainline,
                Ignore = new IgnoreConfig
                {
                    Before = DateTime.Now.AddMinutes(1)
                }
            }).Build();
        fixture.AssertFullSemver("1.0.0", config);
    }

@asbjornu
Copy link
Member

@ibvhefe thanks for the provided test. Does it fail on @rominator1983's fix/IgnoreAllCommits_Issue3074 branch or on GitTools:main?

@ibvhefe
Copy link

ibvhefe commented Sep 29, 2022 via email

@rominator1983
Copy link
Contributor Author

Think I'm gonna check it out then.
Thanks for the feedback and most of all the test

@arturcic arturcic force-pushed the main branch 2 times, most recently from be69f4f to 1826e18 Compare March 4, 2023 13:18
@arturcic arturcic force-pushed the main branch 2 times, most recently from 9801764 to b7a7608 Compare March 4, 2023 13:33
@arturcic
Copy link
Member

@rominator1983 please rebase your PR on top of the current state of the main branch

@arturcic arturcic added the stale label Mar 19, 2023
@rominator1983
Copy link
Contributor Author

Ok guys. I'm gonna rage quit this. My original code is not applicable in any way after you let it sit there silently for half a year.
So I would have to completely dig into that code AGAIN and invest the original amount of time I already put into this once more in order to find out which places would have to be edited.

And since there were a lot of changes upstream I don't exactly know if the original problem does still occur anyways. So...

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

Successfully merging this pull request may close these issues.

[Bug] ignore parameter does not ignore everything
4 participants