Add LexicographicSortedVersion #72

tleed5 · 2024-01-03T06:41:02Z

[SC-67249]

Background

Currently to get around the issue where we need a version number but don't have a good value to use so we work around it by using an Octopus version since that is the most lax version type, an example would be when sourcing scripts from git the only value we have is the git commit hash which lead to the current work around where we make a dummy octopus version and put the commit hash in the release section of the version but this lead to issues where if the git commit hash started with a number that was over the maximum integer value it would cause an overflow exception to be thrown when parsing it.

Solution

Adds a new versioning type LexicographicSortedVersion where the major/minor/patch/revision values are always 0 and the main value is within the release part of the version. The use case for this versioning type would be for cases such as sourcing resources from git where we would use the git commit hash as the input for the version.

It's called unsortable as they cannot be sorted in the same way as Semver e.g. 0.0.1 -> 0.0.2. That said it can still be sorted alphabetically by the release label

Alternatives considered

Updating the regex of OctopusVersionParser to prevent starting numbers from been parsed as the major version but this would break potentially valid versioning schemes as OctopusVersion is based on existing version schemes like Maven which does allow that. See this PR for more details #73

Splitting IVersion interface into two separate interfaces one that extends IVersion and IComparable. But this would lead to a non-trivial amount of work to get this working within Octopus Server and Calamari due to how much the IVersion interface is used and how it is usually expected to be comparable

shortcut-integration · 2024-01-03T06:41:06Z

This pull request has been linked to Shortcut Story #67249: Git sourcing fails if repo version is specific style.

tleed5 · 2024-01-03T06:49:32Z

source/Octopus.Versioning/Unsortable/UnsortableVersion.cs

+            if (!(obj is IVersion objVersion))
+                return -1;
+
+            if (string.Compare(Release.AlphaNumericOnly(), (objVersion.Release ?? string.Empty).AlphaNumericOnly(), StringComparison.Ordinal) != 0)
+                return CompareReleaseLabels(Release.AlphaNumericOnly().Split('.', '-', '_'), (objVersion.Release ?? string.Empty).AlphaNumericOnly().Split('.', '-', '_'));
+
+            return 0;


🤔 Do we get any benefit from this? I thinking we could just return 0 every time

Ah, it sucks that IVersion is also IComparable 🤔

Is it possible to separate the two interfaces? So that IVersion does not directly extend IComparable.
This means that you'd have to go to all existing implementations of IVersion and add IComparable to it explicitly. This should give the benefit that you won't have to implement IComparable methods on Unsortable, which itself seems kind of misleading.

Alternatively, have another interface like ISortableVersion that inherits from IVersion and IComparable. That way a consumer of ISortableVersion knows for sure that whatever it is getting is sortable and can be ordered.

I think it is possible but the effort required might be too great, there is quite a few places around Octopus and Calamari that expect IVersion to sortable e.g. https://github.com/OctopusDeploy/Calamari/blob/b63a7e5ed31644b85a3ade67534eae97b806b055/source/Calamari.Shared/Integration/Certificates/FileSystem/PackageStore.cs#L77 so we would end having to update all of these places well

I think UnsortableVersion is probably a bad name for this since it is technically sortable just only lexicographically and not by the version numbers so maybe we just need to make it obvious how it sorts, compared to the other version types.

@zentron Any thoughts on this?

I agree, I don't like that we call this UnsortableVersion but then sort it.

The way I see it we have 2 options.

Actually, separate out the interfaces as suggested by @kevjt

Rename this version type to something like LexicographicSortedVersion and continue with this approach.

Off the top of my head, I can't think what downsides we would have with using the lexicographic versioning strategy for our commit hashes. We could get some slight performance improvement from not comparing something that can't be compared, but it's probably of minimal impact in the real world, all things considered.

Before moving forward with it, I'd be interested to see a quick spike in how many places would be impacted from a separate sortable interface.

To close this off, I did a quick spike to see how much effort would be required to split the interface and it's a decent amount, there is about 130 different usages of IVersion within Octopus Server and there is a few spots where the usages of IVersion it is always expected to be compared, an example of this would be the PackageCache where a package could be both IVersion and ISortableVersion so we would have to make some changes to functionality and it's hard to get insight to how that change would affect other places in code.

So we are continuing with option 2

tleed5 · 2024-01-08T00:00:22Z

source/Octopus.Versioning/Unsortable/UnsortableVersion.cs

+
+        public VersionFormat Format => VersionFormat.Unsortable;
+
+        public int CompareTo(object obj)


Borrowed the lexicographical sorting bits from OctopusVersion

zentron

Before locking in the lexicographic versioning lets consider how much effort it would be to really go all in with an proper unsorted version.

zentron · 2024-01-09T10:47:40Z

source/Octopus.Versioning.Tests/Unsortable/UnsortableVersionParserTests.cs

+    [TestCase("foobar-qwe ty", "", "")]
+    [TestCase("foobar+123 456", "", "")]
+    [TestCase("foo bar-qwe ty+123 456", "", "")]
+    [TestCase("!foobar", "", "")]


Whats going on with these versions?
Rather than returning empty versions shouldn't we throw with an exception?

This test is using the TryParse method which catches the thrown exception and will return false + an empty version if it fails to parse, this follows the pattern from the other parsers. There is a few other tests in this suite that test the Parse method to see if it throws an exception

zentron · 2024-01-09T11:04:05Z

source/Octopus.Versioning/Unsortable/UnsortableVersion.cs

+            if (!(obj is IVersion objVersion))
+                return -1;
+
+            if (string.Compare(Release.AlphaNumericOnly(), (objVersion.Release ?? string.Empty).AlphaNumericOnly(), StringComparison.Ordinal) != 0)
+                return CompareReleaseLabels(Release.AlphaNumericOnly().Split('.', '-', '_'), (objVersion.Release ?? string.Empty).AlphaNumericOnly().Split('.', '-', '_'));
+
+            return 0;


I agree, I don't like that we call this UnsortableVersion but then sort it.

The way I see it we have 2 options.

Actually, separate out the interfaces as suggested by @kevjt

Rename this version type to something like LexicographicSortedVersion and continue with this approach.

Off the top of my head, I can't think what downsides we would have with using the lexicographic versioning strategy for our commit hashes. We could get some slight performance improvement from not comparing something that can't be compared, but it's probably of minimal impact in the real world, all things considered.

Before moving forward with it, I'd be interested to see a quick spike in how many places would be impacted from a separate sortable interface.

kevjt

Thanks for investigating the options

zentron

Nice one, will get this work over the line eventually!

Added new versioning type - Unsortable

475333b

tleed5 commented Jan 3, 2024

View reviewed changes

tleed5 marked this pull request as draft January 4, 2024 06:16

tleed5 commented Jan 8, 2024

View reviewed changes

tleed5 marked this pull request as ready for review January 8, 2024 00:00

tleed5 mentioned this pull request Jan 8, 2024

Updated OctopusVersionParser regex to ensure delimiters between version numbers #73

Closed

zentron requested changes Jan 9, 2024

View reviewed changes

Rename to LexicographicSortedVersion

48638fd

tleed5 changed the title ~~Add unsortable version type~~ Add LexicographicSortedVersion Jan 12, 2024

kevjt approved these changes Jan 14, 2024

View reviewed changes

zentron approved these changes Jan 15, 2024

View reviewed changes

tleed5 merged commit 08377a1 into main Jan 15, 2024
2 checks passed

tleed5 deleted the tl/add-unsortable-version-type branch January 15, 2024 00:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add LexicographicSortedVersion #72

Add LexicographicSortedVersion #72

tleed5 commented Jan 3, 2024 •

edited

Loading

shortcut-integration bot commented Jan 3, 2024

tleed5 Jan 3, 2024

MissedTheMark Jan 3, 2024

kevjt Jan 8, 2024

tleed5 Jan 8, 2024

zentron Jan 9, 2024

tleed5 Jan 12, 2024

tleed5 Jan 8, 2024

zentron left a comment

zentron Jan 9, 2024

tleed5 Jan 12, 2024

zentron Jan 9, 2024

kevjt left a comment

zentron left a comment


		public VersionFormat Format => VersionFormat.Unsortable;

		public int CompareTo(object obj)

Add LexicographicSortedVersion #72

Add LexicographicSortedVersion #72

Conversation

tleed5 commented Jan 3, 2024 • edited Loading

Background

Solution

Alternatives considered

shortcut-integration bot commented Jan 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zentron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevjt left a comment

Choose a reason for hiding this comment

zentron left a comment

Choose a reason for hiding this comment

tleed5 commented Jan 3, 2024 •

edited

Loading