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.

Sign up for GitHub

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

Jump to bottom

Use Rocket's deb comparator to order artifact versions #429

Merged
merged 3 commits into from
Sep 11, 2019
Merged

Use Rocket's deb comparator to order artifact versions #429

merged 3 commits into from
Sep 11, 2019

Conversation

robfletcher
Copy link
Contributor

No description provided.

robfletcher
robfletcher commented Sep 11, 2019
View reviewed changes
keel-api/src/test/kotlin/com/netflix/spinnaker/keel/rest/ArtifactControllerTests.kt
store(artifact, "fnord-2.1/builds/3")
store(artifact, "fnord-1.0.0-41595c4")
store(artifact, "fnord-2.0.0-608bd90")
store(artifact, "fnord-2.1.0-18ed1dc")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slight concern that version always have to conform to a valid format in order for things to work

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems fine to me... 🤷‍♀

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually this should no longer be a problem once I addressed @cfieber's suggestion to make the comparison null-safe

robfletcher
robfletcher commented Sep 11, 2019
View reviewed changes
...-core-test/src/main/kotlin/com/netflix/spinnaker/keel/persistence/ArtifactRepositoryTests.kt
val version1_2 = "1.2"
val version1 = "keeldemo-0.0.1~dev.8-h8.41595c4"
val version2 = "keeldemo-0.0.1~dev.9-h9.3d2c8ff"
val version3 = "keeldemo-0.0.1~dev.10-h10.1d2d542"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My pathological case

robfletcher
robfletcher commented Sep 11, 2019
View reviewed changes
...-core-test/src/main/kotlin/com/netflix/spinnaker/keel/persistence/ArtifactRepositoryTests.kt
}

test("versions are returned newest first") {
expectThat(subject.versions(artifact1)).containsExactly(version3, version2, version1)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test to see if sorting is correct

robfletcher
robfletcher commented Sep 11, 2019
View reviewed changes
keel-core/src/main/kotlin/com/netflix/spinnaker/keel/persistence/ArtifactRepository.kt
}

fun Collection<String>.sortAppVersion() =
sortedWith(VERSION_COMPARATOR.reversed())
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes this available for use by whatever implementation

cfieber
cfieber approved these changes Sep 11, 2019
View reviewed changes
Copy link
Contributor

@cfieber cfieber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor comments otherwise LGTM

keel-core/src/main/java/com/netflix/rocket/semver/DebianVersionComparator.java Outdated
@@ -0,0 +1,123 @@
package com.netflix.rocket.semver;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thoughts on calling this package com.netflix.rocket.semver.shaded or something so if this becomes a class in its own library we can import without silent conflict?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems legit

keel-core/src/main/kotlin/com/netflix/spinnaker/keel/persistence/ArtifactRepository.kt Outdated

private val debComparator = DebianVersionComparator()

private fun String.toVersion() = AppVersion.parseName(this).version
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if its like everything else in Frigga, AppVersion.parseName probably thoroughly enjoys returning null

I think the usage in compare needs to guard against null args (and maybe support null args) to honour the Comparator contract? unless this is some kotlin take on Comparator that sorts out null already

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's why I had to update the ArtifactControllerTests to use valid-ish appversion strings. Guarding against this is a thoroughly good idea, though.

emjburns
emjburns reviewed Sep 11, 2019
View reviewed changes
keel-core/src/main/java/com/netflix/rocket/semver/shaded/DebianVersionComparator.java
@@ -0,0 +1,123 @@
package com.netflix.rocket.semver.shaded;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this need to be in the "shaded" package?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was @cfieber's suggestion

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ahh I see it now. Cool

@robfletcher robfletcher merged commit e319202 into spinnaker:master Sep 11, 2019
@robfletcher robfletcher deleted the app-version-ordering branch September 12, 2019 14:43
lorin pushed a commit to lorin/keel that referenced this pull request Aug 9, 2021
@SihangYu
Merge pull request spinnaker#429 in SPKR/keel-nflx from sihang-fix-em…
4f583f2 
…oji to master

Squashed commit of the following:

commit 4a7812c6e96da89bd9c8f342e1aad9a9a7af65c4
Author: Sihang Yu <[email protected]>
Date:   Thu Jul 22 10:33:27 2021 -0700

    fix(notification): Use the correct emoji for pluginNotification PASS/FAIL

(cherry picked from commit 2fa25613d1b8596c3adc255fa9ac89af47a48159)
lorin pushed a commit to lorin/keel that referenced this pull request Aug 10, 2021
@SihangYu
Merge pull request spinnaker#429 in SPKR/keel-nflx from sihang-fix-em…
4fd6099 
…oji to master

Squashed commit of the following:

commit 4a7812c6e96da89bd9c8f342e1aad9a9a7af65c4
Author: Sihang Yu <[email protected]>
Date:   Thu Jul 22 10:33:27 2021 -0700

    fix(notification): Use the correct emoji for pluginNotification PASS/FAIL

(cherry picked from commit 2fa25613d1b8596c3adc255fa9ac89af47a48159)
@spinnakerbot spinnakerbot mentioned this pull request Aug 10, 2021
Merged
mergify bot pushed a commit that referenced this pull request Aug 10, 2021
@SihangYu
Merge pull request #429 in SPKR/keel-nflx from sihang-fix-emoji to ma…
ff64d66 
…ster (#1978)

Squashed commit of the following:

commit 4a7812c6e96da89bd9c8f342e1aad9a9a7af65c4
Author: Sihang Yu <[email protected]>
Date:   Thu Jul 22 10:33:27 2021 -0700

    fix(notification): Use the correct emoji for pluginNotification PASS/FAIL

(cherry picked from commit 2fa25613d1b8596c3adc255fa9ac89af47a48159)

Co-authored-by: Sihang Yu <[email protected]>
luispollo pushed a commit to luispollo/keel that referenced this pull request Aug 21, 2021
@SihangYu @luispollo
Merge pull request spinnaker#429 in SPKR/keel-nflx from sihang-fix-em…
4c3e259 
…oji to master

Squashed commit of the following:

commit 4a7812c6e96da89bd9c8f342e1aad9a9a7af65c4
Author: Sihang Yu <[email protected]>
Date:   Thu Jul 22 10:33:27 2021 -0700

    fix(notification): Use the correct emoji for pluginNotification PASS/FAIL

(cherry picked from commit 2fa25613d1b8596c3adc255fa9ac89af47a48159)
@spinnakerbot spinnakerbot mentioned this pull request Aug 21, 2021
Merged
dmart pushed a commit to dmart/keel that referenced this pull request Jul 19, 2022
@SihangYu @dmart
Merge pull request spinnaker#429 in SPKR/keel-nflx from sihang-fix-em…
2eee91f 
…oji to master (spinnaker#1978)

Squashed commit of the following:

commit 4a7812c6e96da89bd9c8f342e1aad9a9a7af65c4
Author: Sihang Yu <[email protected]>
Date:   Thu Jul 22 10:33:27 2021 -0700

    fix(notification): Use the correct emoji for pluginNotification PASS/FAIL

(cherry picked from commit 2fa25613d1b8596c3adc255fa9ac89af47a48159)

Co-authored-by: Sihang Yu <[email protected]>
osoriano pushed a commit to osoriano/keel that referenced this pull request Sep 2, 2023
@SihangYu @osoriano
Merge pull request spinnaker#429 in SPKR/keel-nflx from sihang-fix-em…
2d84a0b 
…oji to master (spinnaker#1978)

Squashed commit of the following:

commit 4a7812c6e96da89bd9c8f342e1aad9a9a7af65c4
Author: Sihang Yu <[email protected]>
Date:   Thu Jul 22 10:33:27 2021 -0700

    fix(notification): Use the correct emoji for pluginNotification PASS/FAIL

(cherry picked from commit 2fa25613d1b8596c3adc255fa9ac89af47a48159)

Co-authored-by: Sihang Yu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emjburns emjburns emjburns approved these changes

@cfieber cfieber Awaiting requested review from cfieber

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@robfletcher @cfieber @emjburns