feat(artifact): introduce SKIPPED status, more data for previous #959

emjburns · 2020-04-02T00:30:31Z

Recently we were talking about artifact statuses. We have an APPROVED status which means "this artifact is ready to be deployed". But, we sometimes have the situation where an artifact is approved for an environment but never deployed to that environment. This is probably because the artifact was superseded by another version of an artifact (both were ready to deploy, but we deployed the latest one).

We also have a PENDING status for artifacts to represent that something might be going on in the future, but we have no idea what. That is calculated like this: we look at all versions we know about, and versions that we have data for in an environment (like APPROVED/CURRENT etc). The versions that we know about but are not in the environment are marked PENDING. This is weird, because some versions are never going to happen (for example: two artifacts are waiting on manual judgment. You approve the latest one. We ignore that other version for ever and ever).

This PR introduces a SKIPPED status which will get set when an artifact is marked as CURRENT in an environment. When an artifact is marked as deployed, we

set the existing CURRENT artifact to PREVIOUS (this pr also updates the promotion_reference column to contain what artifact replaced it)
set any versions that are lower than the existing version and have a status of APPROVED to SKIPPED (for example: 1.1.1, 1.2.1, and 1.3.1 are all approved. 1.2.1 is marked as CURRENT. 1.1.1 would be marked as SKIPPED. 1.3.1 would be untouched)

This will make our existing data a bit wonkey (i.e. every place we now have APPROVED but that's out of date and confusing). It should be straightforward going forward.

This SKIPPED status is also used for pending judgments. That works like this:

we calculate PENDING in the same way as described above
we mark any versions that are < the current version in the environment as SKIPPED

After this PR is merged, we should have a better view of artifacts that will never be deployed.

asher · 2020-04-02T00:42:57Z

promotion_reference has a special meaning currently, see i.e. https://github.com/spinnaker/keel/blob/master/keel-sql/src/main/kotlin/com/netflix/spinnaker/keel/sql/SqlArtifactRepository.kt#L488-L494. Repurposing it this way, without rewriting the existing use will probably break automated artifact version vetoing and rollbacks.

luispollo · 2020-04-02T01:25:20Z

...-core-test/src/main/kotlin/com/netflix/spinnaker/keel/persistence/ArtifactRepositoryTests.kt

+
+          context("there are two approved versions for the environment and the latter was deployed") {
+            before {
+              clock.incrementBy(Duration.ofHours(1))


If you think it's easier to read, you can now do clock.tickHours(1).

luispollo · 2020-04-02T01:26:27Z

keel-core/src/main/kotlin/com/netflix/spinnaker/keel/core/api/PromotionStatus.kt

@@ -1,5 +1,32 @@
 package com.netflix.spinnaker.keel.core.api

 enum class PromotionStatus {
-  PENDING, APPROVED, DEPLOYING, CURRENT, PREVIOUS, VETOED
+  /**
+   * Waiting on constraint evaluation before being approved


Thanks for documenting the enum values! ❤️

keel-sql/src/main/kotlin/com/netflix/spinnaker/keel/sql/SqlArtifactRepository.kt

erikmunson · 2020-04-02T03:08:22Z

😍 fantastic!

Just for my own clarification: does this also modify the behavior of pending in a similar way to approved? Don't know enough about the code to tell fo sure from reading the diff. I know we'd talked about both and couldn't tell if you were thinking about each state separately or also changing that here.

gal-yardeni · 2020-04-02T15:11:39Z

keel-sql/src/main/kotlin/com/netflix/spinnaker/keel/sql/SqlArtifactRepository.kt

+        .where(ENVIRONMENT_ARTIFACT_VERSIONS.ENVIRONMENT_UID.eq(environmentUid))
+        .and(ENVIRONMENT_ARTIFACT_VERSIONS.ARTIFACT_UID.eq(artifact.uid))
+        .and(ENVIRONMENT_ARTIFACT_VERSIONS.PROMOTION_STATUS.eq(APPROVED.name))
+        .and(ENVIRONMENT_ARTIFACT_VERSIONS.ARTIFACT_VERSION.`in`(*approvedButOld.toTypedArray()))


what does the star in *approvedButOld? does? just wondering since I haven't seen this pattern here before

It takes a list of strings and turns it into whatever form sql needs to actually be able to tell whether a string is in a list of strings :D

emjburns · 2020-04-03T20:07:46Z

Updated this PR to address my accidental misuse of the PROMOTION_REFERENCE field and deal with PENDING status.

.../src/main/kotlin/com/netflix/spinnaker/keel/persistence/ApplicationSummaryGenerationTests.kt

gal-yardeni · 2020-04-03T20:27:16Z

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

+  fun isHigher(artifact: DeliveryArtifact, new: String, existing: String): Boolean =
+    artifact.versioningStrategy.comparator.compare(new, existing) < 0
+
+  /**


General question - we are relaying here on the fact that a newer version will get the higher number. is it always the case?

Yes! It is always the case that we are deciding which is the newer version by this comparator, and "higher" version will always be newer. It's one of our core artifact assumptions. It's definitely an assumption, but it's what allows us to do cool artifact things.

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

.../src/main/kotlin/com/netflix/spinnaker/keel/persistence/ApplicationSummaryGenerationTests.kt

keel-core/src/main/kotlin/com/netflix/spinnaker/keel/core/api/EnvironmentSummary.kt

lorin · 2020-04-03T21:05:41Z

The PR description says:

This PR introduces a SKIPPED status which will get set when an artifact is marked as DEPLOYED to an environment.

What does when an artifact is marked as DEPLOYED mean? At first, I thought it was about PromotionStatus, but there's no DEPLOYED enum value.

keel-sql/src/main/kotlin/com/netflix/spinnaker/keel/sql/SqlArtifactRepository.kt

luispollo · 2020-04-03T21:08:47Z

keel-sql/src/main/kotlin/com/netflix/spinnaker/keel/sql/SqlArtifactRepository.kt

+          .and(ENVIRONMENT_ARTIFACT_VERSIONS.ARTIFACT_VERSION.ne(version))
+          .execute()
+        // update any past artifacts that were "APPROVED" to be "SKIPPED
+        //        // because the new version takes precedence


Suggested change

// // because the new version takes precedence

// because the new version takes precedence

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

luispollo · 2020-04-03T21:12:43Z

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

+  /**
+   * Marks a version of an artifact as skipped for an environment, with information on what version skipped it.
+   */
+  fun markAsSkipped(


This function is currently only used in tests. Do you anticipate it being used elsewhere? If not, should probably move it to the test classes.

I anticipate that it might, which is why I kept it here.

.../src/main/kotlin/com/netflix/spinnaker/keel/persistence/memory/InMemoryArtifactRepository.kt

keel-sql/src/main/kotlin/com/netflix/spinnaker/keel/sql/SqlArtifactRepository.kt

lorin

(Some of the comments have been obsoleted by recent changes)

.../src/main/kotlin/com/netflix/spinnaker/keel/persistence/ApplicationSummaryGenerationTests.kt

lorin · 2020-04-03T22:33:23Z

keel-sql/src/main/kotlin/com/netflix/spinnaker/keel/sql/SqlArtifactRepository.kt

-        .and(ENVIRONMENT_ARTIFACT_VERSIONS.PROMOTION_STATUS.eq(CURRENT.name))
-        .and(ENVIRONMENT_ARTIFACT_VERSIONS.ARTIFACT_VERSION.ne(version))
-        .execute()
+      jooq.transaction { config ->


I've noticed that sometimes we use @Transactional annotations and sometimes we call jooq.transaction. Is there some guidance on when to do which?

@Transactional is something that spring does (we use it on the api layer sometimes), and the jooq one is for when you're actually in the sql repository and can control every part of the transaction

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

lorin · 2020-04-03T22:45:34Z

keel-sql/src/main/kotlin/com/netflix/spinnaker/keel/sql/SqlArtifactRepository.kt

@@ -594,6 +617,32 @@ class SqlArtifactRepository(
    }
  }

+  override fun markAsSkipped(


Is this called in application logic yet, or just in tests? I didn't see any reference in application code.

it's just in tests (there's a comment on the interface method for that) I wanted to easily call it in the tests with a separate impl in each repository

keel-core/src/main/kotlin/com/netflix/spinnaker/keel/core/api/PromotionStatus.kt

…PromotionStatus.kt Co-Authored-By: Erik Munson <erik@ipsumcreative.com>

gal-yardeni

Thanks for addressing the comments! lgtm :)

emjburns requested review from asher, robfletcher, lorin, luispollo and gal-yardeni April 2, 2020 00:30

luispollo reviewed Apr 2, 2020

View reviewed changes

keel-sql/src/main/kotlin/com/netflix/spinnaker/keel/sql/SqlArtifactRepository.kt Outdated Show resolved Hide resolved

luispollo reviewed Apr 2, 2020

View reviewed changes

keel-sql/src/main/kotlin/com/netflix/spinnaker/keel/sql/SqlArtifactRepository.kt Outdated Show resolved Hide resolved

gal-yardeni reviewed Apr 2, 2020

View reviewed changes

emjburns added the do not merge label Apr 2, 2020

emjburns added 2 commits April 3, 2020 12:18

feat(artifact): introduce SKIPPED status, more data for previous

43fe5f1

feat(artifacts): dealing with PENDING artifacts

e6334cf

emjburns force-pushed the skipped-artifact-status branch from 15f690e to e6334cf Compare April 3, 2020 19:55

emjburns removed the do not merge label Apr 3, 2020

gal-yardeni reviewed Apr 3, 2020

View reviewed changes

.../src/main/kotlin/com/netflix/spinnaker/keel/persistence/ApplicationSummaryGenerationTests.kt Outdated Show resolved Hide resolved

gal-yardeni reviewed Apr 3, 2020

View reviewed changes

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

lorin reviewed Apr 3, 2020

View reviewed changes

.../src/main/kotlin/com/netflix/spinnaker/keel/persistence/ApplicationSummaryGenerationTests.kt Outdated Show resolved Hide resolved