Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
refactor(artifacts): Extract artifact resolution into ArtifactResolve…
…r class (spinnaker#3377) * refactor(artifacts): Add ArtifactResolver class The ArtifactUtils class handles a lot of functionality and is difficult to understand. Simplify this by creating an ArtifactResolver class to handle resolving artifacts, so that functionality can be removed from ArtifactUtils. One important change in this class is that it does not mutate any of the Artifact or ExpectedArtifact instances it receives; it returns the results of its work (containing new instances) which the caller should then use as appropriate. This will help reduce bugs where the caller is depending on a mutation happening several calls below the call stack, and brings us closer to making Artifact and ExpectedArtifact immutable. * refactor(artifacts): Update callers to use ArtifactResolver This commit updates all artifact resolving code to use the new ArtifactResolver class. A few things of note in the changes: * FindArtifactFromExecutionTaskTest was mocking resolveExpectedArtifacts to return resolvedArtifacts; this was subtly different than the actual function because the actual resolvedArtifacts function mutated the input expectedArtifacts to add a boundArtifact to each. As the resolveExpectedArtifacts method has been moved to a simpler class that we don't need to mock, the test is calling the actual method now and so we need to update the expected result to account for the fact that the resolvedExpectedArtifacts now have a bound artifact. * All of the tests deleted from ArtifactUtilsSpec have equivalent new tests in ArtifactResolverTest which test the new implementation of artifact resolving. * The one changed test in ArtifactUtilsSpec (...sets the bound artifact...) is a result of fixing what was a bug in the prior code. As the prior code was mutating each ExpectedArtifact, we were mutating the expected artifacts on the pipeline itself (and testing that). resolveArtifacts is storing the resolved artifacts in the trigger, under resolvedArtifacts; this is where downstream stages look to find bound artifacts and where we should be testing that the bound artifact is set. The test now checks that the bound artifacts are set in the trigger. * refactor(artifacts): Clean up some stream code The code to map input artifacts in resolveArtifacts() is more complex and verbose than necessary. Simplify it by moving the .orElse before the map operation (which means we don't need to have a nested stream) and also inline a two streams that are used only once. * perf(artifacts): Lazily compute past pipeline artifacts The resolveArtifacts function always passes the prior artifacts to the ArtifactResolver (except when it short-circuits in the event of no expected artifacts). Looking up artifacts from prior executions can be expensive, so ideally we'd defer looking these up until they are actually needed. This change updates the constructor for ArtifactResolver to accept a Supplier of prior artifacts; it will memoize this supplier and only invoke it if the resolution requires looking up prior artifacts (that is, the first time it encounters an expected artifact with usePriorAritfact set to true that does not match one of the current artifacts). This will also allow us in the next commit to remove the short-circuiting for the case where there are no expected artifacts without worrying about negatively affecting performance. * refactor(artifacts): A few minor refactors of resolveArtifacts Replace the List<ExpectedArtifact> with an ImmutableList<> as we never modify it. (And there's a tiny performance benefit that ArtifactResolver.getInstance should short-circuit the copy.) Replace .distinct() collecting to a List<> of receivedArtifacts with directly collecting to toImmutableSet() as it will remove duplicates and return a set whose iteration order is the encounter order in the stream. Remove the short-circuit that returns early if there are no expected artifacts; the ArtifactResolver is a no-op that returns empty lists for resolvedArtifacts and resolvedExpectedArtifacts in that case, so there is no need to special-case it. Replace the LinkedHashSet code that creates a deduplicated list of artifacts in the supplied order with an ImmutableSet that does the same. * fix(artifacts): Fix typo in comment Co-Authored-By: Maggie Neterval <[email protected]> * refactor(artifacts): Make static factory parameters non-null We never actually need currentArtifacts to be null, so remove the nullable annotation. To handle priorArtifacts, add an overload that omits the parameter. Also update the resolveExpectedArtifacts parameter to be non-null. In order to ensure a safe non-null chain of custody, also added Nonnull to the artifacts field in FindArtifactFromExecutionContext, and while there made it immutable. (Also slightly changed the constructor as prior to this the case where both expectedArtifact and expectedArtifacts were null create a singleton array with a null element that was likely to create downstream hard-to-debug issues.) * refactor(artifacts): Improve method order in ArtifactResolver Move resolveExpectedArtifacts above the private methods it uses. * style(artifacts): Add parameter comments for constant parameters Add documentation of the parameter name for all of the usages of the requireUniqueMatches parameter. Co-authored-by: Ethan Rogers <[email protected]> Co-authored-by: Maggie Neterval <[email protected]>
- Loading branch information
1 parent
2e78355
commit db7988f