fix(kubernetes): tolerate an empty manifest in KubernetesDeployManifestOperation #6337
Conversation
…stOperation in case the result of a bake stage is empty. Without this there's: java.lang.NullPointerException at com.netflix.spinnaker.clouddriver.kubernetes.op.manifest.KubernetesDeployManifestOperation.getManifestsFromDescription(KubernetesDeployManifestOperation.java:234) at com.netflix.spinnaker.clouddriver.kubernetes.op.manifest.KubernetesDeployManifestOperation.operate(KubernetesDeployManifestOperation.java:75) at com.netflix.spinnaker.clouddriver.kubernetes.op.manifest.KubernetesDeployManifestOperation.operate(KubernetesDeployManifestOperation.java:48) because KubernetesDeployManifestDescription.getManifestsFromDescription assumes that description.getManifest() never returns null.
| inputManifests = ImmutableList.of(manifest); | ||
|
|
||
| // manifest may be null as well, so check | ||
| if (manifest != null) { |
There was a problem hiding this comment.
Hrmm, wondering if shouldn't log "Warning! Input manifest is null and description is null. Something is incorrect on the deployment most likely!" or similar. I'd not have though BOTH would be null... ALSO... won't this immediately fail below on lin 242 with an NPEsince inputManifests would be null in this check?
There was a problem hiding this comment.
I forget how this all works with description.getManifest() vs description.getManifests(), but apparently inputManifests isn't null in this case, so things work. If this did end up generating a NullPointerException, we'd have seen it. I have a vague memory that @mattgogerly may know something about this, but I think this change is OK. It's at least an improvement if it doesn't fix everything.
There was a problem hiding this comment.
MOSTLY calling out the next line:
inputManifests = inputManifests.stream().filter(Objects::nonNull).collect(Collectors.toList());
inputManifests would be null, triggering an NPE if it got to the point of your null check. ....
There was a problem hiding this comment.
I guess if the inputManifests.isEmpty() would allow this to "work" without an NPE... that may be how this is working...
There was a problem hiding this comment.
Yeah, I figure that's what's happening.
There was a problem hiding this comment.
SIGH I hate "it's working through a glitch" situations... but... approving. I could see a "TOOD" to note to fix this... but as long as it works it works... and tests'll catch it hopefully!
dbyron-sf
deleted the
check-for-null-in-KubernetesDeployManifestOperation-getManifestsFromDescription
branch
in case the result of a bake stage is empty. Without this there's:
because KubernetesDeployManifestDescription.getManifestsFromDescription assumes that description.getManifest() never returns null.