Relax semVers pattern according to Issue #163

[caiusno1]

Changes

• change semVers regex pattern to allow also characters in first part, but restrict to number as first character after "v"
  (From the issue i can't figure out what is the right degree of relaxation, but this solutions seems better to me then the current)
  Pls mention if I understood the relaxation wrong

related Issue: #163

Checklist

• Read the contribution guide and accept the code of conduct
• Readme (updated or not needed)
• Tests (added, updated or not needed)

[codecov]

Codecov Report

Merging #171 into main will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #171   +/-   ##
=======================================
  Coverage   73.50%   73.50%           
=======================================
  Files          17       17           
  Lines         385      385           
  Branches       73       73           
=======================================
  Hits          283      283           
  Misses         92       92           
  Partials       10       10           

Impacted Files	Coverage Δ
src/model/versioning.js	100.00% <100.00%> (ø)

[GabLeRoux]

In fact, semver already provides a regex: https://semver.org/#is-there-a-suggested-regular-expression-regex-to-check-a-semver-string maybe we should use this (and then add the git describe output part on top of that).

[GabLeRoux]

Here are some more test cases we should use:

2.0.0
2.0.0-rc.2
2.0.0-rc.1
1.0.0
1.0.0-beta

These are provided by https://semver.org/

[webbertakken]

The tests do not reflect all new cases, and is a lot more relaxed than just adding one letter to the minor bit.

Either way this is not adhering to semver specification.

A normal version number MUST take the form X.Y.Z where X, Y, and Z are non-negative integers, and MUST NOT contain leading zeroes. X is the major version, Y is the minor version, and Z is the patch version. Each element MUST increase numerically. For instance: 1.9.0 -> 1.10.0 -> 1.11.0.

[GabLeRoux]

thanks for your contribution, I do think it's better too like this. After thinking of it tho, I think the problem is a bit bigger than only the regex. This is a part of the fix, but there's a bigger problem in related issue:

I think this will solve it for most cases, but it won't fix the source of the problem:
github-actions should not fail if a tag doesn't match semver. The code should try to find a semver out of it, but if it fails to do so, it shouldn't prevent us from building.

[webbertakken]

Exactly.

[caiusno1]

Ok, let me try to translate the feedback to tasks (tell me if i am wrong ;-)

• Allow only semVersioning in the regex and no similar cases (it should be the one used, that semver specification suggests)
• Build anyway also when semVers can't be matched (fallback to Custom or None Strategy?)
• Maybe give a warning if SemVers can not match
  Am I right ?

[GabLeRoux]

Ok, let me try to translate the feedback to tasks (tell me if i am wrong ;-)

• Allow only semVersioning in the regex and no similar cases (it should be the one used, that semver specification suggests)
• Build anyway also when semVers can't be matched (fallback to Custom or None Strategy?)
• Maybe give a warning if SemVers can not match
  Am I right ?

I think you are right. Also note that the output we use there comes from git describe's output which provides some relative to last tag output.

I don't know if that's the best way to grab the latest tag, but it does work. I don't exactly know which path we should take here. @webbertakken mentioned about using semver directly, maybe we should also use some git node package to get a list of tags, but I don't know if it would be too heavy.

I'll let @webbertakken determine how it should be implemented ;)

[webbertakken]

So the idea of automatic versioning is that things become easier.

You tag 1.0 and builder uses it as a version like 1.0.0.
You create 5 commits after that tag, and builder will use that as version 1.0.5.

Now in semantic versioning you can have alpha and all, but this is not handled in case of automatic versioning.
It would also create new edge cases because the minor bit is no longer the last part of the version used.

It's as intended that the build fails for that by the way, as there's no logic handling these edge cases, and i've never taken that into account. When no tag has been set it also works. So everything works out of the box until you need more specific rules for your versioning.

A fallback doesn't sound like a good idea, as it's unpredictable and doesn't solve a real problem.

For advanced customised versions like 1.0.1a I would actually recommend just using "custom" strategy instead.
See also the historic discussions from #80 #101 and #105. A lot of consideration has already gone into the current strategy.

[GabLeRoux]

So everything works out of the box until you need more specific rules for your versioning.

My opinion differ a bit here. By default, it should not fail the build because the version system couldn't find a semver compatible version when tags are being used. If we can't give a warning and continue, maybe the semver system should be opt-in. git tags can be used for many things which may not be versions. Assuming that all tags will be semver versions by default feels wrong to me (again, it's an opinion).

I think it should try, but if it fails, it shouldn't use version, but still continue.

[webbertakken]

I don't disagree. We should probably ignore non-semver versions indeed.

[caiusno1]

What is about installing a toggle? Strict Mode => Fail when SemVers could not be matched, None Strict Mode => Only give warnings if SemVers could not be matched. ("In case of doubt ask the user" Principle). This would be the way in between because I think both arguments are valid and important!

[webbertakken]

I think it's an excellent idea. However that would be another option to document and maintain. I'd personally prefer to just make the most favourable option the default, unless there are real use cases that require the second option as well.

I think if we simply ignore non-semantic tags and emit a warning for them, we should be fine. That way it stays simple for everyone and we wouldn't bloat the docs with yet another sub-option.

[caiusno1]

Ok, then I will change the behavior to warn (instead of fail) on non-semVers tags (when I find time :D). Maybe I will also prepare that toggle without exposing it (can then only enabled in code not in config). In that way we can later decide wether it is needed or wether it should be droped bc. of unnecessary complexity. If you are fine with that :).

[webbertakken]

@caiusno1 do you think you will be able to make the last changes?

[caiusno1]

I opened a new merge request hence my approach to the issue is quite different now. See #196

[caiusno1]

Sorry that it took so long. To be honest I forgot it .... and I currently have less time and motivation for programming because of much work currently. @webbertakken

[webbertakken]

Fixed in #196