feat(artifact/decoration): Artifact decoration spinnaker/spinnaker#1348 #202

brujoand · 2017-05-09T10:25:54Z

The motivation here is to enable Artifact decoration for bakes without altering current workflow. I've tried to write this in such a way that it's easy to make the bakeprovider more generic in the future. This also means I had to take some choices so feel free to weigh in. This change gives us the following output on the bake endpoint:

curl -s http://localhost:8087/api/v1/global/bake/e2f492f2-ee4e-4bc3-b2b8-f0e8179625c7 | jq .
{
  "id": "e2f492f2-ee4e-4bc3-b2b8-f0e8179625c7",
  "ami": null,
  "image_name": "myrepo/the-team/ah4234h",
  "artifact": {
    "name": "the-team/mycontainer",
    "type": "docker",
    "version": "1494232370153",
    "reference": "containers.schibsted.io/delivery-team/delivery-dash:ah4234h",
    "metadata": {
      "build_info_url": "https://travis-ci.org/mybuild/53",
      "build_number": "53"
    }
  }
}

WDYT?

jervi · 2017-05-09T11:04:02Z

rosco-core/src/main/groovy/com/netflix/spinnaker/rosco/api/Artifact.groovy

+import groovy.transform.EqualsAndHashCode
+import groovy.transform.ToString
+
+@CompileStatic


Don't use @CompileStatic unless you really have to

jervi · 2017-05-09T11:06:18Z

rosco-core/src/main/groovy/com/netflix/spinnaker/rosco/persistence/RedisBackedBakeStore.groovy

    def bakeRequestJson = mapper.writeValueAsString(bakeRequest)
    def bakeStatusJson = mapper.writeValueAsString(bakeStatus)
    def bakeLogsJson = mapper.writeValueAsString(bakeStatus.logsContent ? [logsContent: bakeStatus.logsContent] : [:])
    def createdTimestampMilliseconds = timeInMilliseconds
    def keyList = ["allBakes", bakeStatus.id, bakeKey, thisInstanceIncompleteBakesKey, lockKey.toString()]
-    def argList = [createdTimestampMilliseconds + "", region, bakeRequestJson, bakeStatusJson, bakeLogsJson, command, roscoInstanceId]
+    def argList = [createdTimestampMilliseconds + "", region, bakeRecipeJson, bakeRequestJson, bakeStatusJson, bakeLogsJson, command, roscoInstanceId]


I know this is existing code, but you could probably change createdTimestampMilliseconds + "" to createdTimestampMilliseconds as String instead (if the casting/conversion is necessary at all?)

duftler

I like the approach and the naming. Nice job.
Just a handful of minor comments.

duftler · 2017-05-13T10:27:30Z

rosco-core/src/main/groovy/com/netflix/spinnaker/rosco/executor/BakePoller.groovy

+            BakeRequest bakeRequest = bakeStore.retrieveBakeRequestById(bakeId)
+            BakeRecipe bakeRecipe = bakeStore.retrieveBakeRecipeById(bakeId)
+
+            if (bakeDetails) {


Seems like the retrieveBakeRequestById() and retrieveBakeRecipeById() calls should also be within this block.

duftler · 2017-05-13T10:29:22Z

rosco-core/src/main/groovy/com/netflix/spinnaker/rosco/persistence/BakeStore.groovy

@@ -34,7 +35,7 @@ interface BakeStore {
   * Store the region, bakeRequest and bakeStatus in association with both the bakeKey and bakeId. If bake key
   * has already been set, return a bakeStatus with that bake's id instead. None of the arguments may be null.
   */
-  public BakeStatus storeNewBakeStatus(String bakeKey, String region, BakeRequest bakeRequest, BakeStatus bakeStatus, String command)
+  public BakeStatus storeNewBakeStatus(String bakeKey, String region, BakeRecipe bakeRecipe, BakeRequest bakeRequest, BakeStatus bakeStatus, String command)


Should probably also update the method comment.

duftler · 2017-05-13T10:33:56Z

...o-core/src/main/groovy/com/netflix/spinnaker/rosco/providers/CloudProviderBakeHandler.groovy

-                                                   parameterMap,
-                                                   finalVarFileName,
-                                                   finaltemplateFilePath)
+    def packerCommand = packerCommandFactory.buildPackerCommand (


Could keep the formatting the same here as it was.

duftler · 2017-05-13T10:34:15Z

...o-core/src/main/groovy/com/netflix/spinnaker/rosco/providers/CloudProviderBakeHandler.groovy

@@ -189,10 +214,11 @@ abstract class CloudProviderBakeHandler {
    def finalVarFileName = bakeRequest.var_file_name ? "$configDir/$bakeRequest.var_file_name" : null
    def baseCommand = getBaseCommand(finalTemplateFileName)



Can drop this extraneous blank line now that the next statement is not a return.

duftler · 2017-05-13T10:34:46Z

rosco-core/src/test/groovy/com/netflix/spinnaker/rosco/executor/BakePollerSpec.groovy

@@ -129,4 +136,72 @@ class BakePollerSpec extends Specification {
      1 * bakeStoreMock.retrieveBakeStatusById(JOB_ID) >> new BakeStatus()
  }

+  void 'Decorate the bakeDetails with an artifact if bake is successfull'() {


s/successfull/successful

ah, I always turn of spellcheck when coding. Because it trips over my variables and methods. I guess it's time to put it back on.

duftler · 2017-05-13T10:42:08Z

rosco-core/src/test/groovy/com/netflix/spinnaker/rosco/executor/BakePollerSpec.groovy

+      1 * bakeStoreMock.updateBakeDetails(bakeDetails)
+  }
+
+  void 'Skip artifact decoration if required data is missing'() {


s/Skip/skip

I don't think this test proves what it says it does. The guard on missing data in completeBake() is on bakeDetails, not bakeRecipe, and the produceArtifactDecorationFrom() method is still called. We just don't verify it here, and the cloudProviderBakeHandlerMock just returns null for it by default.

Yes you're right. I struggled to get this test right, which made me pull out produceArtifactDecorationFrom from BakePoller, so that it was a simple contained method, which would be easier to test. I ment to delete this test and replace it with more accurate testing in CloudProviderBakeHandlerSpec. I'll clean this up.

duftler · 2017-05-13T10:51:17Z

...o-core/src/main/groovy/com/netflix/spinnaker/rosco/providers/CloudProviderBakeHandler.groovy

+   * but it could be useful to override this method for specific providers.
+   */
+  def Artifact produceArtifactDecorationFrom(BakeRequest bakeRequest, BakeRecipe bakeRecipe, Bake bakeDetails, String cloudProvider) {
+    Artifact bakedArtifact = new Artifact (


Don't need the space before the opening parens for the constructor.

duftler · 2017-05-13T11:00:42Z

...re/src/test/groovy/com/netflix/spinnaker/rosco/providers/CloudProviderBakeHandlerTest.groovy

+      producedArtifact == expectedArtifact
+  }
+
+  void 'we should not fail if data is missing for artifact decoration'() {


Could break this down further and use a where: block to prove out each combination of missing data.

duftler · 2017-05-13T11:01:28Z

rosco-web/src/main/groovy/com/netflix/spinnaker/rosco/controllers/BakeryController.groovy

-                                                                 bakeRequest,
-                                                                 newBakeStatus,
-                                                                 jobRequest.tokenizedCommand.join(" "))
+    BakeStatus returnedBakeStatus = bakeStore.storeNewBakeStatus (


Don't see the need to reformat here if just adding the one arg. I can live with it, but leads to more reading in review. Either way, don't need the space before the opening parens.

duftler · 2017-05-13T11:02:55Z

rosco-web/src/main/groovy/com/netflix/spinnaker/rosco/controllers/BakeryController.groovy

-      def packerCommand = cloudProviderBakeHandler.producePackerCommand(region, bakeRequest)
-      def jobRequest = new JobRequest(tokenizedCommand: packerCommand, maskedPackerParameters: cloudProviderBakeHandler.maskedPackerParameters, jobId: bakeRequest.request_id)
+      def bakeRecipe = cloudProviderBakeHandler.produceBakeRecipe(region, bakeRequest)
+      def jobRequest = new JobRequest (


Don't need the space before the opening parens.

I do see that we've been inconsistent with our formatting already. I prefer this style, but can live with it either way.

Yeah, I'm not a fan of the all-the-way-to-the-right indentations, so I was launching a silent rebellion in this PR to check the response. But consistency is even more important so I'll go with the flow on this one. No biggy.

Were you going to undo the reformatting on this one?

brujoand · 2017-05-19T08:12:06Z

Thanks, I've addressed the comments and gotten back in touch with master.

duftler

Thanks for the fixes. Couple of remaining bits and then we'll get it merged.

duftler · 2017-05-20T16:21:32Z

rosco-core/src/test/groovy/com/netflix/spinnaker/rosco/executor/BakePollerSpec.groovy

+      1 * cloudProviderBakeHandlerRegistryMock.lookup(DOCKER_CLOUD_PROVIDER) >> cloudProviderBakeHandlerMock
+      1 * bakeStoreMock.retrieveRegionById(JOB_ID) >> REGION
+      1 * cloudProviderBakeHandlerMock.scrapeCompletedBakeResults(REGION, JOB_ID, LOGS_CONTENT) >> bakeDetails
+      1 * cloudProviderBakeHandlerMock.produceArtifactDecorationFrom(bakeRequest, bakeRecipe, bakeDetails, DOCKER_CLOUD_PROVIDER.toString()) >> bakedArtifact


duftler · 2017-05-20T16:23:00Z

...re/src/test/groovy/com/netflix/spinnaker/rosco/providers/CloudProviderBakeHandlerSpec.groovy

+
+    then:
+    producedArtifact == expectedArtifact
+    producedArtifact.type == expectedArtifact.type


Do these 4 assertions prove something the line just above does not?

My original comments was that you could use @unroll and where: and have one test case with request missing, one with recipe missing, one with details missing, various combinations...

ah, yeah. I completely misunderstood you.

duftler · 2017-05-20T16:24:45Z

rosco-web/src/main/groovy/com/netflix/spinnaker/rosco/controllers/BakeryController.groovy

-      def packerCommand = cloudProviderBakeHandler.producePackerCommand(region, bakeRequest)
-      def jobRequest = new JobRequest(tokenizedCommand: packerCommand, maskedPackerParameters: cloudProviderBakeHandler.maskedPackerParameters, jobId: bakeRequest.request_id)
+      def bakeRecipe = cloudProviderBakeHandler.produceBakeRecipe(region, bakeRequest)
+      def jobRequest = new JobRequest (


Were you going to undo the reformatting on this one?

duftler · 2017-05-20T16:27:53Z

...o-core/src/main/groovy/com/netflix/spinnaker/rosco/providers/CloudProviderBakeHandler.groovy

@@ -132,10 +154,13 @@ abstract class CloudProviderBakeHandler {
   */
  abstract String getTemplateFileName(BakeOptions.BaseImage baseImage)

+


The other methods have just one blank line separating them.

duftler · 2017-05-20T16:28:06Z

...o-core/src/main/groovy/com/netflix/spinnaker/rosco/providers/CloudProviderBakeHandler.groovy

+    return bakedArtifact
+  }
+
+


Extraneous blank line.

duftler

LGTM.

… (spinnaker#202)

jervi approved these changes May 9, 2017

View reviewed changes

brujoand force-pushed the decorate_baked_artifacts branch from 86d6938 to 516d574 Compare May 9, 2017 11:20

duftler requested changes May 13, 2017

View reviewed changes

brujoand force-pushed the decorate_baked_artifacts branch 3 times, most recently from 2cd8b73 to 64fbc60 Compare May 19, 2017 08:00

duftler reviewed May 20, 2017

View reviewed changes

feat(artifact/decoration): Artifact decoration spinnaker/spinnaker#1348

7aec0c5

brujoand force-pushed the decorate_baked_artifacts branch from 64fbc60 to 7aec0c5 Compare May 22, 2017 13:40

duftler approved these changes May 22, 2017

View reviewed changes

duftler merged commit 072cfa3 into spinnaker:master May 23, 2017

opsmxdemo pushed a commit to OpsMx/rosco that referenced this pull request Sep 15, 2020

feat(artifact/decoration): Artifact decoration spinnaker/spinnaker#1348…

e2627d0

… (spinnaker#202)

opsmxdemo pushed a commit to OpsMx/rosco that referenced this pull request Sep 16, 2020

feat(artifact/decoration): Artifact decoration spinnaker/spinnaker#1348…

f96f624

… (spinnaker#202)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(artifact/decoration): Artifact decoration spinnaker/spinnaker#1348 #202

feat(artifact/decoration): Artifact decoration spinnaker/spinnaker#1348 #202

brujoand commented May 9, 2017

jervi May 9, 2017

jervi May 9, 2017

duftler left a comment

duftler May 13, 2017

duftler May 13, 2017

duftler May 13, 2017

duftler May 13, 2017

duftler May 13, 2017

brujoand May 19, 2017

duftler May 13, 2017

duftler May 13, 2017

brujoand May 19, 2017

duftler May 13, 2017

duftler May 13, 2017

brujoand May 19, 2017

duftler May 13, 2017

duftler May 13, 2017

duftler May 13, 2017

brujoand May 19, 2017

duftler May 20, 2017

brujoand commented May 19, 2017

duftler left a comment

duftler May 20, 2017

duftler May 20, 2017

duftler May 20, 2017

brujoand May 22, 2017

duftler May 20, 2017

duftler May 20, 2017

duftler May 20, 2017

duftler left a comment

		@@ -189,10 +214,11 @@ abstract class CloudProviderBakeHandler {
		def finalVarFileName = bakeRequest.var_file_name ? "$configDir/$bakeRequest.var_file_name" : null
		def baseCommand = getBaseCommand(finalTemplateFileName)

		@@ -132,10 +154,13 @@ abstract class CloudProviderBakeHandler {
		*/
		abstract String getTemplateFileName(BakeOptions.BaseImage baseImage)

feat(artifact/decoration): Artifact decoration spinnaker/spinnaker#1348 #202

feat(artifact/decoration): Artifact decoration spinnaker/spinnaker#1348 #202

Conversation

brujoand commented May 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duftler 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

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

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

brujoand commented May 19, 2017

duftler 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

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

duftler left a comment

Choose a reason for hiding this comment