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] The configuration JSON schemas use the wrong properties for default values and assign bad regex patterns #3739

Closed
BinToss opened this issue Oct 27, 2023 · 9 comments · Fixed by #3749
Labels
Milestone

Comments

@BinToss
Copy link
Contributor

BinToss commented Oct 27, 2023

Describe the bug

Some properties in GitVersion.configuration.json are used for the wrong purpose (pattern where default should be used) or are assigned bad values (human-readable date-time formats are assigned to regex-only fields).

Were these mistakes made by a schema generator?

image

Definitions using pattern when they shouldn't, assign literal strings with no regex syntax, or are annotated with the wrong type(s):

  • pattern regex string which must resolve to a DateTime object. This makes no sense. In the first place, this string must be usable as a parameter for System.Runtime.Serialization.DateTimeFormat(string formatString).
    • commit-date-format
  • pattern regex string is optionally used for asserting the property's value. The default annotation should be used instead.
    • major-version-bump-message
    • minor-version-bump-message
    • no-bump-message
    • patch-version-bump-message
    • tag-number-pattern (only in 5.12 config schema)

See this branch for manual changes

RFC 3339 Max Value (no offset): 9999-12-31t23:59:59z
RFC 3339 Min Value (no offset): 0000-01-01t00:00:00z

RedHat's vscode-yaml somehow follows RFC 3339's spec. It includes case-insensitivity for the 't' and `z' (both of which are mandatory) and can process the various acceptable formats for the optional offset numbers.

Expected Behavior

The *-version-bump-message fields should...
...not use the pattern property expect any regex string and use the default annotation field.

Actual Behavior

The *-version-bump-message fields expect regex-like values which perfectly match the regex pattern in the pattern field which is currently set the default values of each field.
If the fields' values are different than the default values, YAML/JSON validators declare it an error.

Possible Fix

scratch that. I'm making a PR. While fixing the pattern-default issues, I discovered the regex patterns for date-time fields are useless.

"major-version-bump-message": {
"format": "regex",
"pattern": "\u0027\\\u002Bsemver:\\s?(breaking|major)\u0027",
"description": "The regular expression to match commit messages with to perform a major version increment. Default set to \u0027\\\u002Bsemver:\\s?(breaking|major)\u0027",
"type": "string"
},

    "major-version-bump-message": {
      "format": "regex",
-      "pattern": "\u0027\\\u002Bsemver:\\s?(breaking|major)\u0027",
+      "default": "\u0027\\\u002Bsemver:\\s?(breaking|major)\u0027",
      "description": "The regular expression to match commit messages with to perform a major version increment. Default set to \u0027\\\u002Bsemver:\\s?(breaking|major)\u0027",
      "type": "string"
    },

"minor-version-bump-message": {
"format": "regex",
"pattern": "\u0027\\\u002Bsemver:\\s?(feature|minor)\u0027",
"description": "The regular expression to match commit messages with to perform a minor version increment. Default set to \u0027\\\u002Bsemver:\\s?(feature|minor)\u0027",
"type": "string"
},

    "minor-version-bump-message": {
      "format": "regex",
-      "pattern": "\u0027\\\u002Bsemver:\\s?(feature|minor)\u0027",
+      "default": "\u0027\\\u002Bsemver:\\s?(feature|minor)\u0027",
      "description": "The regular expression to match commit messages with to perform a minor version increment. Default set to \u0027\\\u002Bsemver:\\s?(feature|minor)\u0027",
      "type": "string"
    },

Steps to Reproduce

In an editor with JSON/YAML validation (e.g. VSCode), create GitVersion.yml and write minor-version-bump-message: "^(feat)(\\([\\w\\s-]*\\))?:" to it.

winget install Microsoft.VisualStudioCode # install Visual Studio Code, then...
start 'vscode:extension/redhat.vscode-yaml' # install YAML Language Server extension
'minor-version-bump-message: "^(feat)(\\([\\w\\s-]*\\))?:"' > GitVersion.yml
code ./GitVersion.yml
# 'trust workspace' to allow the yaml extension to run

Context

VSCode complains that custom version bump message patterns don't match the "required" patterns for their respective fields.

Your Environment

  • Operating System: Windows 11 Pro Version 22H2 (OS Build 22621.2428)
  • VSCode: ["1.83.1", "f1b07bd25dfad64b0167beb15359ae573aecd2cc", "x64"]
  • redhat.vscode-yaml: 1.14.0
  • GitVersion.MSBuild: 6.0.0-beta.3
  • GitVersion.configuration.json: 6.0
@BinToss BinToss added the bug label Oct 27, 2023
@asbjornu
Copy link
Member

Good question. @arturcic, any idea what might be causing this?

@arturcic
Copy link
Member

Most probably we will need to improve the description of the mentioned properties so that the generated schema is correct.

@arturcic
Copy link
Member

@BinToss
Copy link
Contributor Author

BinToss commented Oct 27, 2023

As an aside, commit-date-format's pattern could be rewritten in regex to properly allow "real" dates (leap days, months with 30 days) and prohibit impossible dates (30th/31st of February; 31st of March/April/June/September/November). I started on it at https://regexr.com/7mahd. Those exceptions-to-the-rule will be complex to solve.

@BinToss
Copy link
Contributor Author

BinToss commented Nov 2, 2023

I've noticed an issue with JsonPropertyPatternAttribute that will require a bit more refactoring.
It was implemented as an amalgamation of

// e.g. "555-1212" or "(888)555-1212", and NOT "(800)FLOWERS"
"type": "string",
"pattern": "^(\\([0-9]{3}\\))?[0-9]{3}-[0-9]{4}$"

and

// e.g. \+semver:\s?(breaking|major)
"type": "string",
"format": "regex"

The former implies the value of the string-type property should conform to the specified Regular Expression pattern which may be asserted by a validator or language server.
The latter implies the value of the string-type property should be a RegEx pattern.
And the following implies a string-type property that can be parsed as a DateTime object:

// e.g. "2018-11-13T20:20:39+00:00"
"type": "string",
"format": "date-time"

Additionally, the date-time format may be asserted to follow RFC 3339, section 5.6, a subset of ISO8601 format.

I'll apply the ObsoleteAttribute to the PatternFormat enum with an explanation and refactor code to separate the usage of patterns and formats.

@BinToss
Copy link
Contributor Author

BinToss commented Nov 4, 2023

I'm discussing how to use JSON's default keyword over here

@BinToss
Copy link
Contributor Author

BinToss commented Nov 4, 2023

Why are default values–specifically, the ones identical in both GitFlow and GitHubFlow–not defined/assigned in the GitVersionConfiguration? For now, we can add attributes to hint at the supposed default values, but these attribute's values could instead be inferred from initializers...or compile-time constant properties i.e. each configuration type would have public const of the configuration type with their default values.

@BinToss
Copy link
Contributor Author

BinToss commented Nov 5, 2023

For the sake of making the PR easier on me, I'll define default values in ConfigurationConstants and move existing defaults to it.
If this is undesired, I'll revert the commit(s).

@arturcic arturcic added this to the 6.x milestone Nov 20, 2023
@arturcic arturcic modified the milestones: 6.x, 6.0.0-beta.4 Dec 12, 2023
@arturcic
Copy link
Member

🎉 This issue has been resolved in version 6.0.0-beta.4 🎉
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
3 participants