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

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

Merged
merged 3 commits into from
Sep 11, 2019
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -59,9 +59,9 @@ internal class ArtifactControllerTests {
val artifact = DeliveryArtifact("fnord", DEB)
with(artifactRepository) {
register(artifact)
store(artifact, "fnord-1.0/builds/1")
store(artifact, "fnord-2.0/builds/2")
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

}

val request = get("/artifacts/${artifact.name}/${artifact.type}")
Expand All @@ -71,9 +71,9 @@ internal class ArtifactControllerTests {
.andExpect(status().isOk)
.andExpect(content().string(
"""---
|- "fnord-2.1/builds/3"
|- "fnord-2.0/builds/2"
|- "fnord-1.0/builds/1"
|- "fnord-2.1.0-18ed1dc"
|- "fnord-2.0.0-608bd90"
|- "fnord-1.0.0-41595c4"
""".trimMargin()
))
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,9 @@ abstract class ArtifactRepositoryTests<T : ArtifactRepository> : JUnit5Minutests
artifacts = setOf(artifact1, artifact2),
environments = setOf(environment1, environment2)
)
val version1_0 = "1.0"
val version1_1 = "1.1"
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

}

fun tests() = rootContext<Fixture<T>> {
Expand All @@ -57,7 +57,7 @@ abstract class ArtifactRepositoryTests<T : ArtifactRepository> : JUnit5Minutests

test("registering a new version throws an exception") {
expectThrows<NoSuchArtifactException> {
subject.store(artifact1, version1_0)
subject.store(artifact1, version1)
}
}

Expand Down Expand Up @@ -87,20 +87,32 @@ abstract class ArtifactRepositoryTests<T : ArtifactRepository> : JUnit5Minutests

context("an artifact version already exists") {
before {
subject.store(artifact1, version1_0)
subject.store(artifact1, version1)
}

test("registering the same version is a no-op") {
val result = subject.store(artifact1, version1_0)
val result = subject.store(artifact1, version1)
expectThat(result).isFalse()
expectThat(subject.versions(artifact1)).hasSize(1)
}

test("registering a new version adds it to the list") {
val result = subject.store(artifact1, version1_1)
val result = subject.store(artifact1, version2)

expectThat(result).isTrue()
expectThat(subject.versions(artifact1)).containsExactly(version1_1, version1_0)
expectThat(subject.versions(artifact1)).containsExactly(version2, version1)
}
}

context("sorting is consistent") {
before {
listOf(version1, version2, version3).forEach {
subject.store(artifact1, it)
}
}

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

}
}
}
Expand All @@ -117,7 +129,7 @@ abstract class ArtifactRepositoryTests<T : ArtifactRepository> : JUnit5Minutests
}

test("versions are not considered successfully deployed") {
setOf(version1_0, version1_1, version1_2).forEach {
setOf(version1, version2, version3).forEach {
expectThat(subject.wasSuccessfullyDeployedTo(manifest, artifact1, it, environment1.name))
.isFalse()
}
Expand All @@ -126,72 +138,72 @@ abstract class ArtifactRepositoryTests<T : ArtifactRepository> : JUnit5Minutests

context("a version has been promoted to an environment") {
before {
subject.approveVersionFor(manifest, artifact1, version1_0, environment1.name)
subject.approveVersionFor(manifest, artifact1, version1, environment1.name)
}

test("the approved version for that environment matches") {
expectThat(subject.latestVersionApprovedIn(manifest, artifact1, environment1.name))
.isEqualTo(version1_0)
.isEqualTo(version1)
}

test("the version is not considered successfully deployed yet") {
expectThat(subject.wasSuccessfullyDeployedTo(manifest, artifact1, version1_0, environment1.name))
expectThat(subject.wasSuccessfullyDeployedTo(manifest, artifact1, version1, environment1.name))
.isFalse()
}

test("promoting the same version again returns false") {
expectCatching {
subject.approveVersionFor(manifest, artifact1, version1_0, environment1.name)
subject.approveVersionFor(manifest, artifact1, version1, environment1.name)
}
.succeeded()
.isFalse()
}

test("promoting a new version returns true") {
expectCatching {
subject.approveVersionFor(manifest, artifact1, version1_1, environment1.name)
subject.approveVersionFor(manifest, artifact1, version2, environment1.name)
}
.succeeded()
.isTrue()
}

context("the version is marked as successfully deployed") {
before {
subject.markAsSuccessfullyDeployedTo(manifest, artifact1, version1_0, environment1.name)
subject.markAsSuccessfullyDeployedTo(manifest, artifact1, version1, environment1.name)
}

test("the version is now considered successfully deployed") {
expectThat(subject.wasSuccessfullyDeployedTo(manifest, artifact1, version1_0, environment1.name))
expectThat(subject.wasSuccessfullyDeployedTo(manifest, artifact1, version1, environment1.name))
.isTrue()
}

context("a new version is promoted to the same environment") {
before {
subject.approveVersionFor(manifest, artifact1, version1_1, environment1.name)
subject.approveVersionFor(manifest, artifact1, version2, environment1.name)
}

test("the latest approved version changes") {
expectThat(subject.latestVersionApprovedIn(manifest, artifact1, environment1.name))
.isEqualTo(version1_1)
.isEqualTo(version2)
}

test("the version is not considered successfully deployed yet") {
expectThat(subject.wasSuccessfullyDeployedTo(manifest, artifact1, version1_1, environment1.name))
expectThat(subject.wasSuccessfullyDeployedTo(manifest, artifact1, version2, environment1.name))
.isFalse()
}

context("the new version is marked as successfully deployed") {
before {
subject.markAsSuccessfullyDeployedTo(manifest, artifact1, version1_1, environment1.name)
subject.markAsSuccessfullyDeployedTo(manifest, artifact1, version2, environment1.name)
}

test("the old version is still considered successfully deployed") {
expectThat(subject.wasSuccessfullyDeployedTo(manifest, artifact1, version1_0, environment1.name))
expectThat(subject.wasSuccessfullyDeployedTo(manifest, artifact1, version1, environment1.name))
.isTrue()
}

test("the new version is also considered successfully deployed") {
expectThat(subject.wasSuccessfullyDeployedTo(manifest, artifact1, version1_1, environment1.name))
expectThat(subject.wasSuccessfullyDeployedTo(manifest, artifact1, version2, environment1.name))
.isTrue()
}
}
Expand All @@ -200,33 +212,33 @@ abstract class ArtifactRepositoryTests<T : ArtifactRepository> : JUnit5Minutests

context("a version of a different artifact is promoted to the environment") {
before {
subject.approveVersionFor(manifest, artifact2, version1_2, environment1.name)
subject.approveVersionFor(manifest, artifact2, version3, environment1.name)
}

test("the approved version of the original artifact remains the same") {
expectThat(subject.latestVersionApprovedIn(manifest, artifact1, environment1.name))
.isEqualTo(version1_0)
.isEqualTo(version1)
}

test("the approved version of the new artifact matches") {
expectThat(subject.latestVersionApprovedIn(manifest, artifact2, environment1.name))
.isEqualTo(version1_2)
.isEqualTo(version3)
}
}

context("a different version of the same artifact is promoted to another environment") {
before {
subject.approveVersionFor(manifest, artifact1, version1_1, environment2.name)
subject.approveVersionFor(manifest, artifact1, version2, environment2.name)
}

test("the approved version in the original environment is unaffected") {
expectThat(subject.latestVersionApprovedIn(manifest, artifact1, environment1.name))
.isEqualTo(version1_0)
.isEqualTo(version1)
}

test("the approved version in the new environment matches") {
expectThat(subject.latestVersionApprovedIn(manifest, artifact1, environment2.name))
.isEqualTo(version1_1)
.isEqualTo(version2)
}
}
}
Expand Down
5 changes: 5 additions & 0 deletions keel-core/keel-core.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,14 @@ dependencies {
api("de.danielbechler:java-object-diff")
api("org.springframework:spring-context")
api("org.springframework.boot:spring-boot-autoconfigure")
api("com.netflix.frigga:frigga")

testImplementation(project(":keel-test"))
testImplementation(project (":keel-core-test"))
testImplementation("io.strikt:strikt-jackson")
testImplementation("dev.minutest:minutest")

testImplementation("org.assertj:assertj-core")
testImplementation("org.junit.jupiter:junit-jupiter-api")
testImplementation("org.junit.jupiter:junit-jupiter-params")
}
Original file line number Diff line number Diff line change
@@ -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


import java.util.Comparator;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

public class DebianVersionComparator implements Comparator<String> {
private static final String PRERELEASE_SPLIT = "~";
private static final String DOT_SEPARATOR = "\\.";
private static final String PRERELEASE_SEPARATOR = "[.+]";

// separate out the upstream version (everything before the last hyphen) from the debian version
// (everything after the last hyphen)
private static final Pattern upstreamVersionDebianVersionPattern = Pattern.compile("(.*)-(.*)");
private static final Pattern netflixDebianVersionPattern = Pattern.compile("h(\\d+)\\.[0-9a-f]+");

@Override
public int compare(String s0, String s1) {
if (s0.equals(s1)) {
return 0;
}

Matcher m0 = upstreamVersionDebianVersionPattern.matcher(s0);
Matcher m1 = upstreamVersionDebianVersionPattern.matcher(s1);

if (m0.matches() && m1.matches()) {
int versionCompare = compareVersionPart(m0.group(1), m1.group(1));
if (versionCompare == 0) {
return compareBuildPart(m0.group(2), m1.group(2));
}
return versionCompare;
}

return compareVersionPart(s0, s1);
}

private int compareVersionPart(String s0, String s1) {
String[] mainPrerelease0 = s0.split(PRERELEASE_SPLIT);
String[] mainPrerelease1 = s1.split(PRERELEASE_SPLIT);
int mainComparison = compareMainPart(mainPrerelease0[0], mainPrerelease1[0]);
if (mainComparison == 0) {
if (mainPrerelease0.length == mainPrerelease1.length && mainPrerelease0.length == 2) {
String[] prerelease0 = mainPrerelease0[1].split(PRERELEASE_SEPARATOR);
String[] prerelease1 = mainPrerelease1[1].split(PRERELEASE_SEPARATOR);
for (int i = 0; i < prerelease0.length; i++) {
if (i >= prerelease1.length) {
if (prerelease0[i].equals("dev")) {
return -1;
} else {
return 1;
}
}
boolean isNumeric0 = isNumeric(prerelease0[i]);
final boolean isNumeric1 = isNumeric(prerelease1[i]);
if (isNumeric0 && isNumeric1) {
int diff = Integer.parseInt(prerelease0[i]) - Integer.parseInt(prerelease1[i]);
if (diff != 0) {
return diff;
}
}
if (!isNumeric0 && !isNumeric1) {
int diff = prerelease0[i].compareTo(prerelease1[i]);
if (diff != 0) {
return diff;
}
}
}
if (prerelease1.length > prerelease0.length) {
if (prerelease1[prerelease0.length].equals("dev")) {
return 1;
} else {
return -1;
}
}
}
return mainPrerelease1.length - mainPrerelease0.length;
}
return mainComparison;
}

/**
* Compare the main parts of debian versions given 1.2.3~rc.1-h1.abcdef, 1.2.3 is the main part
*
* @param s0 String
* @param s1 String
* @return negative number if s0 less than s1, 0 if s0 equal to s1, positive number if s0 greater
* than s1
*/
private int compareMainPart(String s0, String s1) {
String[] mainParts0 = s0.split(DOT_SEPARATOR);
String[] mainParts1 = s1.split(DOT_SEPARATOR);
for (int i = 0; i < mainParts0.length; i++) {
if (i >= mainParts1.length) {
return 1;
}
int diff = Integer.parseInt(mainParts0[i]) - Integer.parseInt(mainParts1[i]);
if (diff != 0) {
return diff;
}
}
return mainParts0.length - mainParts1.length;
}

private static boolean isNumeric(String strNum) {
try {
Integer.parseInt(strNum);
} catch (NumberFormatException | NullPointerException nfe) {
return false;
}
return true;
}

private int compareBuildPart(String s0, String s1) {
Matcher m0 = netflixDebianVersionPattern.matcher(s0);
Matcher m1 = netflixDebianVersionPattern.matcher(s1);

if (m0.matches() && m1.matches()) {
return Integer.parseInt(m0.group(1)) - Integer.parseInt(m1.group(1));
}

return s0.compareTo(s1);
}
}
Original file line number Diff line number Diff line change
@@ -1,8 +1,12 @@
package com.netflix.spinnaker.keel.persistence

import com.netflix.frigga.ami.AppVersion
import com.netflix.rocket.semver.shaded.DebianVersionComparator
import com.netflix.spinnaker.keel.api.ArtifactType
import com.netflix.spinnaker.keel.api.DeliveryArtifact
import com.netflix.spinnaker.keel.api.DeliveryConfig
import org.slf4j.LoggerFactory
import org.springframework.util.comparator.NullSafeComparator

interface ArtifactRepository {

Expand Down Expand Up @@ -65,6 +69,27 @@ interface ArtifactRepository {
)
}

private val VERSION_COMPARATOR: Comparator<String> = object : Comparator<String> {
override fun compare(s1: String, s2: String) =
debComparator.compare(s1.toVersion(), s2.toVersion())

private val debComparator = NullSafeComparator(DebianVersionComparator(), true)

private fun String.toVersion() = AppVersion
.parseName(this)
?.version
.also {
if (it == null) {
log.warn("Unparseable artifact version \"{}\" encountered", it)
}
}

private val log by lazy { LoggerFactory.getLogger(javaClass) }
}

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


class NoSuchArtifactException(name: String, type: ArtifactType) :
RuntimeException("No $type artifact named $name is registered") {
constructor(artifact: DeliveryArtifact) : this(artifact.name, artifact.type)
Expand Down
Loading