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(GitVersion.Core, GitVersion.Schema): improve implementation of Types and JSON keywords in GitVersion.Configuration.json #3749

Merged
merged 16 commits into from
Dec 8, 2023

Conversation

BinToss
Copy link
Contributor

@BinToss BinToss commented Nov 2, 2023

Description

Related Issue

Closes #3739

Motivation and Context

While editing a GitVerison.yml in one of my projects, VSCode's redhat.vscode-yaml extension highlighted the values of some fields as errors, stating that the values (regex patterns) was invalidated by the regex pattern required by the schema (GitVersion.configuration

How Has This Been Tested?

  1. Searched for issues in the generated schemas and took note of what was wrong or potentially wrong.
  2. Modified those schemas and renamed them so they would not be overwritten when regenerated. The regenerated schemas should be identical to or similar in effect to the modified schemas.
  3. Created several GitVersion.yml with valid (which the schemas mark as invalid) and invalid values (to ensure they remain invalid). Added a header comment to instruct my IDE's YAML language server to use a local path to the newly generated schema file instead of the remote, online path e.g. # yaml-language-server: $schema=../schemas/6.0/generatedschema.json

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.

@arturcic
Copy link
Member

arturcic commented Nov 2, 2023

@BinToss just wanted to let you know while you're working on this one, the schema files are generated.

The way we run generate them is using the Schema project https://github.com/GitTools/GitVersion/tree/main/src/GitVersion.Schema.

You can locally run it using the following commands:

dotnet build build/CI.sln --configuration=Release
dotnet run/docs.dll --target=PublishDocs --force

So changing the schema files manually is not an option

@arturcic
Copy link
Member

arturcic commented Nov 2, 2023

@BinToss please run dotnet format ./src/ --exclude **/AddFormats/ --verify-no-changes from the root, this should fix the formatting

@BinToss BinToss changed the title Fix-schemas Fix improper use of pattern and other JSON keywords in GitVersion.Configuration.json Nov 5, 2023
@BinToss BinToss changed the title Fix improper use of pattern and other JSON keywords in GitVersion.Configuration.json fix(GitVersion.Core, GitVersion.Schema): improve implementation of Types and JSON keywords in GitVersion.Configuration.json Nov 5, 2023
@BinToss
Copy link
Contributor Author

BinToss commented Nov 5, 2023

dotnet build build/CI.sln --configuration=Release
dotnet run/docs.dll --target=PublishDocs --force

The first command runs successfully.
The second command results in the following error:

> dotnet run/docs.dll --target=PublishDocs --force
Tool gitreleasemanager.tool is already installed, with required version.
Tool wyam2 is already installed, with required version.

----------------------------------------
Setup
----------------------------------------
Could not execute because the specified command or file was not found.
Possible reasons for this include:
  * You misspelled a built-in dotnet command.
  * You intended to execute a .NET program, but dotnet-C:/Repos/BinToss/GitTools.GitVersion/tools/gitversion/gitversion.dll does not exist.
  * You intended to run a global tool, but a dotnet-prefixed executable with this name could not be found on the PATH.

----------------------------------------
TearDown
----------------------------------------
Starting Teardown...
Target:               PublishDocs
Build Agent:          Local
OS:                   Windows
Pull Request:         False
Repository Name:
Original Repository:  False
Branch Name:          fix-schemas
Main Branch:          False
Support Branch:       False
Tagged:               False
IsStableRelease:      False
IsTaggedPreRelease:   False
IsInternalPreRelease: False
Finished running tasks.
Error: GitVersion: Process returned an error (exit code 1).

But I was able to generate some schemas by running GitVersion.Schema directly via New-Item -Type Directory -Force schemas/tmp && dotnet run --project .\src\GitVersion.Schema\ --framework net7.0 --OutputDirectory $PWD/schemas --Version tmp

dotnet format ./src/ --exclude **/AddFormats/ --verify-no-changes

This runs with warnings, but completes successfully. All the warnings are caused by the tests targeting .NETFramework instead of net6.0 or later.

Msbuild failed when processing the file 'C:\Repos\BinToss\GitTools.GitVersion\src\GitVersion.MsBuild.Tests\GitVersion.MsBuild.Tests.csproj' with message: C:\Repos\BinToss\GitTools.GitVersion\src\GitVersion.MsBuild.Tests\GitVersion.MsBuild.Tests.csproj: (0, 0): Package 'Microsoft.Build 17.7.2' was restored using '.NETFramework,Version=v4.6.1, .NETFramework,Version=v4.6.2, .NETFramework,Version=v4.7, .NETFramework,Version=v4.7.1, .NETFramework,Version=v4.7.2, .NETFramework,Version=v4.8, .NETFramework,Version=v4.8.1' instead of the project target framework 'net6.0'. This package may not be fully compatible with your project.
Msbuild failed when processing the file 'C:\Repos\BinToss\GitTools.GitVersion\src\GitVersion.MsBuild.Tests\GitVersion.MsBuild.Tests.csproj' with message: C:\Repos\BinToss\GitTools.GitVersion\src\GitVersion.MsBuild.Tests\GitVersion.MsBuild.Tests.csproj: (0, 0): Package 'Microsoft.IO.Redist 6.0.0' was restored using '.NETFramework,Version=v4.6.1, .NETFramework,Version=v4.6.2, .NETFramework,Version=v4.7, .NETFramework,Version=v4.7.1, .NETFramework,Version=v4.7.2, .NETFramework,Version=v4.8, .NETFramework,Version=v4.8.1' instead of the project target framework 'net6.0'. This package may not be fully compatible with your project.
Msbuild failed when processing the file 'C:\Repos\BinToss\GitTools.GitVersion\src\GitVersion.MsBuild.Tests\GitVersion.MsBuild.Tests.csproj' with message: C:\Repos\BinToss\GitTools.GitVersion\src\GitVersion.MsBuild.Tests\GitVersion.MsBuild.Tests.csproj: (0, 0): Package 'Microsoft.Build 17.7.2' was restored using '.NETFramework,Version=v4.6.1, .NETFramework,Version=v4.6.2, .NETFramework,Version=v4.7, .NETFramework,Version=v4.7.1, .NETFramework,Version=v4.7.2, .NETFramework,Version=v4.8, .NETFramework,Version=v4.8.1' instead of the project target framework 'net6.0'. This package may not be fully compatible with your project.
Msbuild failed when processing the file 'C:\Repos\BinToss\GitTools.GitVersion\src\GitVersion.MsBuild.Tests\GitVersion.MsBuild.Tests.csproj' with message: C:\Repos\BinToss\GitTools.GitVersion\src\GitVersion.MsBuild.Tests\GitVersion.MsBuild.Tests.csproj: (0, 0): Package 'Microsoft.IO.Redist 6.0.0' was restored using '.NETFramework,Version=v4.6.1, .NETFramework,Version=v4.6.2, .NETFramework,Version=v4.7, .NETFramework,Version=v4.7.1, .NETFramework,Version=v4.7.2, .NETFramework,Version=v4.8, .NETFramework,Version=v4.8.1' instead of the project target framework 'net6.0'. This package may not be fully compatible with your project.
Msbuild failed when processing the file 'C:\Repos\BinToss\GitTools.GitVersion\src\GitVersion.MsBuild.Tests\GitVersion.MsBuild.Tests.csproj' with message: C:\Repos\BinToss\GitTools.GitVersion\src\GitVersion.MsBuild.Tests\GitVersion.MsBuild.Tests.csproj: (0, 0): Package 'Microsoft.Build 17.7.2' was restored using '.NETFramework,Version=v4.6.1, .NETFramework,Version=v4.6.2, .NETFramework,Version=v4.7, .NETFramework,Version=v4.7.1, .NETFramework,Version=v4.7.2, .NETFramework,Version=v4.8, .NETFramework,Version=v4.8.1' instead of the project target framework 'net6.0'. This package may not be fully compatible with your project.
Msbuild failed when processing the file 'C:\Repos\BinToss\GitTools.GitVersion\src\GitVersion.MsBuild.Tests\GitVersion.MsBuild.Tests.csproj' with message: C:\Repos\BinToss\GitTools.GitVersion\src\GitVersion.MsBuild.Tests\GitVersion.MsBuild.Tests.csproj: (0, 0): Package 'Microsoft.IO.Redist 6.0.0' was restored using '.NETFramework,Version=v4.6.1, .NETFramework,Version=v4.6.2, .NETFramework,Version=v4.7, .NETFramework,Version=v4.7.1, .NETFramework,Version=v4.7.2, .NETFramework,Version=v4.8, .NETFramework,Version=v4.8.1' instead of the project target framework 'net6.0'. This package may not be fully compatible with your project.
Msbuild failed when processing the file 'C:\Repos\BinToss\GitTools.GitVersion\src\GitVersion.MsBuild.Tests\GitVersion.MsBuild.Tests.csproj' with message: C:\Repos\BinToss\GitTools.GitVersion\src\GitVersion.MsBuild.Tests\GitVersion.MsBuild.Tests.csproj: (0, 0): Package 'Microsoft.Build 17.7.2' was restored using '.NETFramework,Version=v4.6.1, .NETFramework,Version=v4.6.2, .NETFramework,Version=v4.7, .NETFramework,Version=v4.7.1, .NETFramework,Version=v4.7.2, .NETFramework,Version=v4.8, .NETFramework,Version=v4.8.1' instead of the project target framework 'net6.0'. This package may not be fully compatible with your project.
Msbuild failed when processing the file 'C:\Repos\BinToss\GitTools.GitVersion\src\GitVersion.MsBuild.Tests\GitVersion.MsBuild.Tests.csproj' with message: C:\Repos\BinToss\GitTools.GitVersion\src\GitVersion.MsBuild.Tests\GitVersion.MsBuild.Tests.csproj: (0, 0): Package 'Microsoft.IO.Redist 6.0.0' was restored using '.NETFramework,Version=v4.6.1, .NETFramework,Version=v4.6.2, .NETFramework,Version=v4.7, .NETFramework,Version=v4.7.1, .NETFramework,Version=v4.7.2, .NETFramework,Version=v4.8, .NETFramework,Version=v4.8.1' instead of the project target framework 'net6.0'. This package may not be fully compatible with your project.

@BinToss
Copy link
Contributor Author

BinToss commented Nov 11, 2023

JsonScherma.Net.Generation 3.5.0 will implement a classes to support the default JSON keyword. I'm waiting for that version to publish.

EDIT: Oh, it was published three days ago.

@BinToss
Copy link
Contributor Author

BinToss commented Nov 14, 2023

What are the arguments for/against assigning default values to constants in GitVersion.Configuration.ConfigurationConstants?

What about also assigning those constants to their corresponding properties in the Configuration classes under GitVersion.Configuration? e.g. the preexisting ConfigurationConstants.DefaultVersionInBranchPattern could be assigned to the property GitVersionConfiguration.VersionInBranchPattern (though it would be redundant in that particular case).
Otherwise, I'm also considering a Get or Set accessor that checks for a null value and instead returns/assigns the default for the property.

Additionally, if a default in GitFlow and GitHubFlow is the same, should it be moved to ConfigurationConstants?

If everything goes smoothly, I should have this PR ready by Thursday.

@BinToss
Copy link
Contributor Author

BinToss commented Nov 17, 2023

[What are the arguments for/against] assigning [Configuration constants] to their corresponding properties in the Configuration classes under GitVersion.Configuration?

If this is undesired, I'll remove 4746dba

Additionally, if a default in GitFlow and GitHubFlow is the same, should it be moved to ConfigurationConstants?

This set of changes has not yet been committed.

Now...how should I backport these fixes/features to 5.12?

@arturcic
Copy link
Member

Once we get these changes merged into main, I will manually port them to 5.x, but only to generate the schema and then I will include the schema for 5.x in the main branch

@BinToss
Copy link
Contributor Author

BinToss commented Nov 18, 2023

I managed to get build.ps1 working properly. Which is nice. Now I can see that adding the JsonPropertyDefaultAttribute and related classes had no effect on the schemas despite calling DefaultIntent. I'll try to figure that out and a few schema validation false-positives over the weekend.

@arturcic
Copy link
Member

@BinToss please rebase your changes on main branch. We moved most of the configuration code to a separate project

@BinToss
Copy link
Contributor Author

BinToss commented Nov 21, 2023

Rebase done.

During the weekend, I didn't have as much time to look into the aforementioned issues as I'd hoped. I should have an opening tomorrow night to investigate.

@BinToss
Copy link
Contributor Author

BinToss commented Nov 21, 2023

Do the tests need to be updated or is there something wrong with my changes? Some of these assert exceptions shouldn't have been caused by my changes.
e.g.

Shouldly.ShouldAssertException : result.OutputVariables.FullSemVer
should be
"1.0.4-PullRequest5.3"
but was
"1.0.4-PullRequest.3"

@BinToss
Copy link
Contributor Author

BinToss commented Nov 25, 2023

I hadn't known I needed to add more calls to AttributeHandler.AddHandler at
https://github.com/BinToss/GitTools.GitVersion/blob/c5da3c5106f8b7791a8beeb5c0bb8f19cd26195c/src/GitVersion.Schema/Program.cs#L19-L20


We're re-implementing attributes and handlers already provided under namespace Json.Schema.Generation.
Is this redundancy intended to prevent GitVersion.Core from referencing the JsonSchema.Net.Generation package when trimming is unavailable? Or can this redundancy be removed and can GitVersion.Core depend on the attributes provided by package JsonSchema.Net.Generation?

@arturcic
Copy link
Member

I don't remember exactly why I had to add those attributes but I suspect that that was the case. We try to remove any dependency from the GitVersion.Core. But taking in consideration we recently moved the configuration code to its own project I guess you can try to use the JsonSchema.Net.Generation version, and add a dependency on that package to the GitVersion.Configuration project

@BinToss
Copy link
Contributor Author

BinToss commented Nov 25, 2023

Also, the commits-before property (src/GitVersion.Configuration/IgnoreConfiguration.cs#L10, GitVersion.Configuration.IgnoreConfiguration.Before) has its type DateTimeOffset? (Nullable<DateTimeOffset>) generated in the schema as the following:

    "nullableOfDateTimeOffset": {
      "format": "date-time",
      "description": "Commits before this date will be ignored. Format: yyyy-MM-ddTHH:mm:ss.",
      "type": [
        "object",
        "null"
      ],
      "properties": {
        "date": {
          "$ref": "#/$defs/dateTime"
        },
        "date-time": {
          "$ref": "#/$defs/dateTime"
        },
        "day": {
          "$ref": "#/$defs/integer"
        },
        "day-of-week": {
          "enum": [
            "Sunday",
            "Monday",
            "Tuesday",
            "Wednesday",
            "Thursday",
            "Friday",
            "Saturday"
          ],
          "readOnly": true
        },
        "day-of-year": {
          "$ref": "#/$defs/integer"
        },
        "hour": {
          "$ref": "#/$defs/integer"
        },
        "local-date-time": {
          "$ref": "#/$defs/dateTime"
        },
        "microsecond": {
          "$ref": "#/$defs/integer"
        },
        "millisecond": {
          "$ref": "#/$defs/integer"
        },
        "minute": {
          "$ref": "#/$defs/integer"
        },
        "month": {
          "$ref": "#/$defs/integer"
        },
        "nanosecond": {
          "$ref": "#/$defs/integer"
        },
        "offset": {
          "$ref": "#/$defs/timeSpan"
        },
        "second": {
          "$ref": "#/$defs/integer"
        },
        "ticks": {
          "$ref": "#/$defs/integer1"
        },
        "time-of-day": {
          "$ref": "#/$defs/timeSpan"
        },
        "total-offset-minutes": {
          "$ref": "#/$defs/integer"
        },
        "utc-date-time": {
          "$ref": "#/$defs/dateTime"
        },
        "utc-ticks": {
          "$ref": "#/$defs/integer1"
        },
        "year": {
          "$ref": "#/$defs/integer"
        }
      }
    },

i.e. a nullable object with properties instead of a nullable string.

I'm tempted to [JsonIgnore] IgnoreConfiguration.Before and add IgnoreConfiguration.BeforeString as an easy workaround. Unless you'd prefer Before to be refactored more...extensively.

EDIT: the deed is done

@BinToss
Copy link
Contributor Author

BinToss commented Dec 4, 2023

Tests failing locally:

  • GitVersion.Configuration.Tests.Init.InitScenarios.CanSetVersion():
    • Blame: 7c03c3e
    • Reason: The test expects some nullable properties to be omitted from the GitVersion.yml created for the test. Therefore, those properties are intended to be assigned their defaults after a configuration file is deserialized. If a property is missing from a deserialized configuration object, then it is implicitly null and should be assigned the "default" when the object is cast to an EffectiveConfiguration
    • Solution: revert/remove the commit -OR- remove init expressions for the offending properties in GitVersionConfiguration, BranchConfiguration, and IgnoreConfiguration.
  • GitVersion.Core.Tests.GitVersionExecutorTests.CacheKeyForWorktree:
    • Reason: Test will fail if temporary directory from previous operations still exists and contains a .git subdirectory in an invalid (partially deleted?) state.
    • Workaround: Manually deleting the temporary directory clears the issue. It did not recur in following test runs.
    • Stack Trace:
      Exception has occurred: CLR/LibGit2Sharp.RepositoryNotFoundException
      Exception thrown: 'LibGit2Sharp.RepositoryNotFoundException' in LibGit2Sharp.dll: 'Path 'C:\Users\NoahR\AppData\Local\Temp\GitVersion' doesn't point at a valid Git repository or workdir.'
         at LibGit2Sharp.Core.Proxy.git_repository_open(String path)
         at LibGit2Sharp.Repository..ctor(String path, RepositoryOptions options, RepositoryRequiredParameter requiredParameter)
         at LibGit2Sharp.Repository..ctor(String path)
         at GitVersion.GitRepositoryInfo.GitRepoHasMatchingRemote(String possiblePath, String targetUrl) in C:\Repos\BinToss\GitTools.GitVersion\src\GitVersion.LibGit2Sharp\Git\GitRepositoryInfo.cs:line 105
      

…lt` annotation in schemas

Only constant values can be passed to attributes, but an attribute's constructor can convert a parameter to the desired Type.
…n from `pattern`

They are not mutually-inclusive. They have different purposes and can be used without the other.
also, update property descriptions and add comments
…faultLabelNumberPattern

And fix pattern inconsistency. It was failing tests.
Switches should not have values and some keys' values should not be enclosed with double-quotes. Both were leading to fatal errors.
…nd recommend running build.ps1 with "build,BuildPrepare"
…zing Nullable<DateTimeOffset>

Nullable types seem to ignore the types' built-in JSON converters
i.e. DateTimeOffset <=> string, but DateTimeOffset? <=> object or null
@BinToss
Copy link
Contributor Author

BinToss commented Dec 4, 2023

GitHub check CI / Test / windows-latest - net8.0 (pull_request) failed during step [Unit Test].
However, this is due to an exception thrown after the tests succeeded.
https://github.com/GitTools/GitVersion/actions/runs/7085093622/job/19280754082?pr=3749#step:4:2304


  Results File: D:/a/GitVersion/GitVersion/artifacts/test-results/GitVersion.Core.Tests.net8.0.results.xml
  
  Passed!  - Failed:     0, Passed:   613, Skipped:     2, Total:   615, Duration: 40 s - GitVersion.Core.Tests.dll (net8.0)

  Calculating coverage result...
  C:\Users\runneradmin\.nuget\packages\coverlet.msbuild\6.0.0\build\coverlet.msbuild.targets(72,5): error : Unable to read beyond the end of the stream. [D:\a\GitVersion\GitVersion\src\GitVersion.Core.Tests\GitVersion.Core.Tests.csproj::TargetFramework=net8.0]
  C:\Users\runneradmin\.nuget\packages\coverlet.msbuild\6.0.0\build\coverlet.msbuild.targets(72,5): error :    at System.IO.BinaryReader.InternalRead(Int32 numBytes) [D:\a\GitVersion\GitVersion\src\GitVersion.Core.Tests\GitVersion.Core.Tests.csproj::TargetFramework=net8.0]
  C:\Users\runneradmin\.nuget\packages\coverlet.msbuild\6.0.0\build\coverlet.msbuild.targets(72,5): error :    at System.IO.BinaryReader.ReadInt32() [D:\a\GitVersion\GitVersion\src\GitVersion.Core.Tests\GitVersion.Core.Tests.csproj::TargetFramework=net8.0]
  C:\Users\runneradmin\.nuget\packages\coverlet.msbuild\6.0.0\build\coverlet.msbuild.targets(72,5): error :    at Coverlet.Core.Coverage.CalculateCoverage() in /_/src/coverlet.core/Coverage.cs:line 414 [D:\a\GitVersion\GitVersion\src\GitVersion.Core.Tests\GitVersion.Core.Tests.csproj::TargetFramework=net8.0]
  C:\Users\runneradmin\.nuget\packages\coverlet.msbuild\6.0.0\build\coverlet.msbuild.targets(72,5): error :    at Coverlet.Core.Coverage.GetCoverageResult() in /_/src/coverlet.core/Coverage.cs:line 161 [D:\a\GitVersion\GitVersion\src\GitVersion.Core.Tests\GitVersion.Core.Tests.csproj::TargetFramework=net8.0]
  C:\Users\runneradmin\.nuget\packages\coverlet.msbuild\6.0.0\build\coverlet.msbuild.targets(72,5): error :    at Coverlet.MSbuild.Tasks.CoverageResultTask.Execute() in /_/src/coverlet.msbuild.tasks/CoverageResultTask.cs:line 86 [D:\a\GitVersion\GitVersion\src\GitVersion.Core.Tests\GitVersion.Core.Tests.csproj::TargetFramework=net8.0]

Edit: I needed to regenerate the schema, anyway. Maybe this exception was a transient issue.

@BinToss
Copy link
Contributor Author

BinToss commented Dec 4, 2023

@arturcic All checks are passing

@arturcic
Copy link
Member

arturcic commented Dec 4, 2023

@BinToss I will hopefully have a look today

@arturcic arturcic merged commit 2739468 into GitTools:main Dec 8, 2023
129 checks passed
Copy link
Contributor

mergify bot commented Dec 8, 2023

Thank you @BinToss for your contribution!

@BinToss
Copy link
Contributor Author

BinToss commented Mar 27, 2024

@arturcic The 5.12 schemas in main never received the fixes.
image

@arturcic
Copy link
Member

that is because the schema generation from code was implemented in v6, that hopefully soon we can have GA release.

I tried to generate the 5.12 version long time ago, but that is a challenging think to do as 5.x is not maintained anymore, so I'd say you can start using v6 version and the latest beta from 6.x series

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.

[Bug] The configuration JSON schemas use the wrong properties for default values and assign bad regex patterns
2 participants