fix(kubernetes/artifacts): Support for legacy Clouddriver's artifact replacement logic #5513

cristhian-castaneda · 2021-09-08T15:30:03Z

This change adds support for this breaking change #4874.

We added an optional flag to enable the old behavior

kubernetes:
  artifact-binding:
    docker-image: match-name-only

This is mainly to help current users that rely on the old behavior.

…binding

spinnakerbot · 2021-09-08T15:35:17Z

The following commits need their title changed:

0481fe5: Merge remote-tracking branch 'origin/master' into fix/support-legacy-binding

Please format your commit title into the form:

<type>(<scope>): <subject>, e.g. fix(kubernetes): address NPE in status check

This allows us to easily generate changelogs & determine semantic version numbers when cutting releases. You can read more about commit conventions here.

german-muzquiz

I see a potential race condition where this new flag may not work depending on the order in which spring creates the beans: if any spring bean that imports Replacer is created before ReplacerFactory bean is created, when the runtime processes the imports it will invoke the static method of ReplacerFactory, which hasn't yet read the value of the config file and uses the default of false.

This problem is because Replacer is a bunch of static objects and the config flag can only be read from instance fields.

Another approach would be to move using the config flag until the filter is applied rather than when creating the replacers. This means that Replacer would need to have two instances of replacePathSupplier: one standard and one legacy, but which one is used is determined until the replace call. There's a single point of usage in ArtifactReplacer, so we can add the boolean flag to the ArtifactReplacer constructor and populate it from the nearest spring bean that can load it.

Another comment is to change the config setting from artifacts.legacy-binding.enabled to kubernetes.artifact-binding.docker-image, with possible values match-name-and-tag (default) and match-name-only (which is the legacy behavior). In this way we're prepared if we need to add more settings related with how artifact binding works, for example if kubernetes.artifact-binding.enabled is ever implemented globally.

cristhian-castaneda · 2021-09-14T18:44:44Z

...src/main/java/com/netflix/spinnaker/clouddriver/kubernetes/op/handler/KubernetesHandler.java

  protected KubernetesHandler() {
    this.artifactReplacer = new ArtifactReplacer(artifactReplacers());
  }

+  protected void setDockerImageBinding(String dockerImageBinding) {


This method was defined so the concrete class can assign the value from config properties.

german-muzquiz · 2021-09-14T20:28:38Z

...src/main/java/com/netflix/spinnaker/clouddriver/kubernetes/op/handler/KubernetesHandler.java

@@ -45,10 +45,20 @@

  private final ArtifactReplacer artifactReplacer;

+  protected String dockerImageBinding;


Have you tried to inject the value here with @Value instead of injecting in every subclass? In theory it should work

Yeah, you are right, it works, I did some test with this in my first approach but didn't work, never tried that in this approach.

cristhian-castaneda · 2021-09-15T22:14:14Z

@Mergifyio backport release-1.25.x

…replacement logic (#5513) * fix(artifacts): support legacy artifact binding. * fix(artifacts): add comment for unit test. * fix(artifacts): delete unnecessary dependency * fix(artifacts): add comment in param * fix(artifacts): fix import * fix(artifacts): update comment. * fix(artifacts): refactor factory and fix unit test * fix(artifacts): fix typo * fix(artifacts): avoid race condition for static context * fix(artifacts): fix typo * fix(artifacts): run spotlessApply * fix(artifacts): fix NPE * fix(artifacts): delete unnecessary dependency * fix(artifacts): use @value annotation within abstract class * fix(artifacts): delete unnecessary imports * fix(artifacts): run spotlessApply Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> (cherry picked from commit 18e53a0)

mergify · 2021-09-15T22:15:00Z

Command backport release-1.25.x: success

Backports have been created

#5531 fix(kubernetes/artifacts): Support for legacy Clouddriver's artifact replacement logic (backport #5513) has been created for branch release-1.25.x

link108 · 2021-09-16T23:31:01Z

@Mergifyio backport release-1.26.x release-1.27.x

…replacement logic (#5513) * fix(artifacts): support legacy artifact binding. * fix(artifacts): add comment for unit test. * fix(artifacts): delete unnecessary dependency * fix(artifacts): add comment in param * fix(artifacts): fix import * fix(artifacts): update comment. * fix(artifacts): refactor factory and fix unit test * fix(artifacts): fix typo * fix(artifacts): avoid race condition for static context * fix(artifacts): fix typo * fix(artifacts): run spotlessApply * fix(artifacts): fix NPE * fix(artifacts): delete unnecessary dependency * fix(artifacts): use @value annotation within abstract class * fix(artifacts): delete unnecessary imports * fix(artifacts): run spotlessApply Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> (cherry picked from commit 18e53a0)

mergify · 2021-09-16T23:31:56Z

Command backport release-1.26.x release-1.27.x: success

Backports have been created

#5536 fix(kubernetes/artifacts): Support for legacy Clouddriver's artifact replacement logic (backport #5513) has been created for branch release-1.26.x

#5537 fix(kubernetes/artifacts): Support for legacy Clouddriver's artifact replacement logic (backport #5513) has been created for branch release-1.27.x

…replacement logic (backport #5513) (#5536) Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Cristhian Castaneda <ccastanedarivera@gmail.com>

…replacement logic (backport #5513) (#5537) Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Cristhian Castaneda <ccastanedarivera@gmail.com>

…replacement logic (backport #5513) (#5540) * fix(kube/test): Use kind instead of k3s (#5421) * fix(kube/test): Use kind instead of k3s * fix(kube/test): Added README * fix(kube/test): Enabled cache * fix(kube/test): Delete test cluster after all tests finish (cherry picked from commit 807d18e) * chore(deps): update kork to 7.107.0, use new maven coordinates (#5303) (cherry picked from commit 43e3d42) * chore(build): use version 8.14.0 of spinnaker-gradle-project to stop looking in jcenter (#5447) since it no longer exists, and causes builds (e.g. https://github.com/spinnaker/clouddriver/pull/5435/checks?check_run_id=3109445523) to fail. * fix(kubernetes/artifacts): Support for legacy Clouddriver's artifact replacement logic (#5513) * fix(artifacts): support legacy artifact binding. * fix(artifacts): add comment for unit test. * fix(artifacts): delete unnecessary dependency * fix(artifacts): add comment in param * fix(artifacts): fix import * fix(artifacts): update comment. * fix(artifacts): refactor factory and fix unit test * fix(artifacts): fix typo * fix(artifacts): avoid race condition for static context * fix(artifacts): fix typo * fix(artifacts): run spotlessApply * fix(artifacts): fix NPE * fix(artifacts): delete unnecessary dependency * fix(artifacts): use @value annotation within abstract class * fix(artifacts): delete unnecessary imports * fix(artifacts): run spotlessApply Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> (cherry picked from commit 18e53a0) * fix(kube/test): Use kind instead of k3s (#5421) * fix(kube/test): Use kind instead of k3s * fix(kube/test): Added README * fix(kube/test): Enabled cache * fix(kube/test): Delete test cluster after all tests finish (cherry picked from commit 807d18e) * fix(kube/test): Use kind instead of k3s (#5421) * fix(kube/test): Use kind instead of k3s * fix(kube/test): Added README * fix(kube/test): Enabled cache * fix(kube/test): Delete test cluster after all tests finish (cherry picked from commit 807d18e) * fix(kube/test): Use kind instead of k3s (#5421) * fix(kube/test): Use kind instead of k3s * fix(kube/test): Added README * fix(kube/test): Enabled cache * fix(kube/test): Delete test cluster after all tests finish (cherry picked from commit 807d18e) * fix(kube/test): Use kind instead of k3s (#5421) * fix(kube/test): Use kind instead of k3s * fix(kube/test): Added README * fix(kube/test): Enabled cache * fix(kube/test): Delete test cluster after all tests finish (cherry picked from commit 807d18e) Co-authored-by: German Muzquiz <35276119+german-muzquiz@users.noreply.github.com> Co-authored-by: Daniel Peach <daniel.peach@armory.io> Co-authored-by: David Byron <82477955+dbyron-sf@users.noreply.github.com>

…replacement logic (backport #5513) (#5531) Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Cristhian Castaneda <ccastanedarivera@gmail.com> Co-authored-by: Cameron Motevasselani <cmotevasselani@gmail.com>

cristhian-castaneda added 7 commits September 7, 2021 17:57

fix(artifacts): support legacy artifact binding.

483c5c0

fix(artifacts): add comment for unit test.

5f89e3f

Merge remote-tracking branch 'origin/master' into fix/support-legacy-…

0481fe5

…binding

fix(artifacts): delete unnecessary dependency

290da9b

fix(artifacts): add comment in param

a486432

fix(artifacts): fix import

5a0b8aa

fix(artifacts): update comment.

e148a20

cristhian-castaneda added 2 commits September 8, 2021 20:08

fix(artifacts): refactor factory and fix unit test

2b053da

fix(artifacts): fix typo

253d856

cristhian-castaneda marked this pull request as ready for review September 9, 2021 01:13

cristhian-castaneda requested a review from german-muzquiz as a code owner September 9, 2021 01:13

german-muzquiz reviewed Sep 9, 2021

View reviewed changes

cristhian-castaneda added 5 commits September 14, 2021 11:44

fix(artifacts): avoid race condition for static context

89f0b84

fix(artifacts): fix typo

6368650

fix(artifacts): run spotlessApply

49ca66c

fix(artifacts): fix NPE

9c65dbc

fix(artifacts): delete unnecessary dependency

346e58c

cristhian-castaneda commented Sep 14, 2021

View reviewed changes

cristhian-castaneda requested a review from german-muzquiz September 14, 2021 18:45

german-muzquiz reviewed Sep 14, 2021

View reviewed changes

cristhian-castaneda added 3 commits September 14, 2021 17:56

fix(artifacts): use @value annotation within abstract class

36e62d1

fix(artifacts): delete unnecessary imports

bfa47ba

fix(artifacts): run spotlessApply

32b6276

german-muzquiz approved these changes Sep 15, 2021

View reviewed changes

german-muzquiz added the ready to merge Approved and ready for a merge label Sep 15, 2021

Merge branch 'master' into fix/support-legacy-binding

52126b4

mergify bot added the auto merged Merged automatically by a bot label Sep 15, 2021

mergify bot merged commit 18e53a0 into spinnaker:master Sep 15, 2021

spinnakerbot added the target-release/1.28 label Sep 15, 2021

cristhian-castaneda deleted the fix/support-legacy-binding branch September 15, 2021 19:30

link108 added the backport-candidate Add to PRs to designate release branch patch candidates. label Sep 15, 2021

mergify bot mentioned this pull request Sep 15, 2021

fix(kubernetes/artifacts): Support for legacy Clouddriver's artifact replacement logic (backport #5513) #5531

Merged

mergify bot mentioned this pull request Sep 16, 2021

fix(kubernetes/artifacts): Support for legacy Clouddriver's artifact replacement logic (backport #5513) #5536

Merged

mergify bot mentioned this pull request Sep 16, 2021

fix(kubernetes/artifacts): Support for legacy Clouddriver's artifact replacement logic (backport #5513) #5537

Merged

link108 removed the backport-candidate Add to PRs to designate release branch patch candidates. label Sep 17, 2021

cristhian-castaneda mentioned this pull request Sep 23, 2021

fix(kubernetes/artifacts): Support for legacy Clouddriver's artifact replacement logic (backport #5513) #5540

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(kubernetes/artifacts): Support for legacy Clouddriver's artifact replacement logic #5513

fix(kubernetes/artifacts): Support for legacy Clouddriver's artifact replacement logic #5513

cristhian-castaneda commented Sep 8, 2021 •

edited

spinnakerbot commented Sep 8, 2021

german-muzquiz left a comment

cristhian-castaneda Sep 14, 2021

german-muzquiz Sep 14, 2021

cristhian-castaneda Sep 14, 2021

cristhian-castaneda commented Sep 15, 2021

mergify bot commented Sep 15, 2021

link108 commented Sep 16, 2021

mergify bot commented Sep 16, 2021

		@@ -45,10 +45,20 @@

		private final ArtifactReplacer artifactReplacer;

		protected String dockerImageBinding;

fix(kubernetes/artifacts): Support for legacy Clouddriver's artifact replacement logic #5513

fix(kubernetes/artifacts): Support for legacy Clouddriver's artifact replacement logic #5513

Conversation

cristhian-castaneda commented Sep 8, 2021 • edited

spinnakerbot commented Sep 8, 2021

german-muzquiz left a comment

Choose a reason for hiding this comment

cristhian-castaneda Sep 14, 2021

Choose a reason for hiding this comment

german-muzquiz Sep 14, 2021

Choose a reason for hiding this comment

cristhian-castaneda Sep 14, 2021

Choose a reason for hiding this comment

cristhian-castaneda commented Sep 15, 2021

mergify bot commented Sep 15, 2021

link108 commented Sep 16, 2021

mergify bot commented Sep 16, 2021

cristhian-castaneda commented Sep 8, 2021 •

edited