fix(artifacts): fix NPE when expected artifact is null #4846
Conversation
| @@ -120,6 +120,9 @@ public ResolveResult resolveExpectedArtifacts(Iterable<ExpectedArtifact> expecte | |||
| ImmutableList.Builder<ExpectedArtifact> boundExpectedArtifacts = ImmutableList.builder(); | |||
|
|
|||
| for (ExpectedArtifact expectedArtifact : expectedArtifacts) { | |||
| if (expectedArtifact == null) { | |||
There was a problem hiding this comment.
eh? Why would an expected artifact be null? Is this due to clouddriver failing image resolution and deciding to return null instead of an error?
There was a problem hiding this comment.
It is not supposed to be null but looks like someone got NPE somehow(ref: spinnaker/spinnaker#7004). We will investigate the reason but I believe allowing NPE in any case is bad, so this PR is to address those edge cases.
There was a problem hiding this comment.
Im fairly certain it's due to clouddriver image resolution. Instead of failing and returning an error, it returns null for some odd reason. We may opt to fix that instead of the null check here which should be invalid.
This is important cause if we skip the artifact, things may not resolve properly. Like instead of using the expected artifact it will use the default artifact of the pipeline if configured as such.
So I think we need to investigate the why here first
There was a problem hiding this comment.
100% on root cause... if it's a groovy/java change, if it's some OTHER library issue (e.g. we've seen spring upgrades change SPeL empty string stuff), etc. good to fix THAT vs. try to wrap it.
This is to address the issue : spinnaker/spinnaker#7004
While resolving expected artifact NullPointerException is thrown if the artifact is null.
Added a test to demonstrate the issue and provided the fix which identifies null artifact and throws InvalidRequestException which is a SpinnakerException.