Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(dependencies): Upgrade Spring Boot to 2.2.1 #3333

Merged
merged 9 commits into from
Dec 17, 2019

Conversation

pdelagrave
Copy link

Adjusting code for Jedis 3, Jackson 2.10 upgrades.
See spinnaker/spinnaker#5134
Dependent on spinnaker/kork#419 and spinnaker/keiko#72 for a Spring Boot dependency upgrade

Pierre Delagrave added 3 commits December 6, 2019 12:25
…erged

TemplateMerge.mergeDistinct() was merging 2 list of different types into one. Upcoming Jackson library update  would have broken compilation of TemplatedPipelineModelMutator if left unmodified.
Adjusting code for Jedis 3, Jackson 2.10 upgrades.
…tifact`

Artifact resolution wasn't always persisting `pipeline.expectedArtifacts.boundArtifact` depending on the expectedArtifact's type when passed to `ArtifactResolver.resolveArtifacts()`.
Only `ExpectedArtifact` would get their `boundArtifact` updated; if a Map was used then a copy of that Map would be updated instead and this isn't returned to the caller.
The update of Jackson to 2.10 made it so that `boundArtifact` would always be updated on a copy of the passed `expectedArtifact` no matter its type, this commit is an attempt to fix both problems.
A better fix would be to make it clear what type is accepted and expected, but that's a much larger refactor.
Copy link
Member

@robzienert robzienert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like there's some breakages here?

@@ -165,7 +164,7 @@ private void applyConfigurations(
TemplateMerge.mergeDistinct(
pipelineTemplateObjectMapper.convertValue(
pipeline.get("expectedArtifacts"),
new TypeReference<List<ExpectedArtifact>>() {}),
new TypeReference<List<HashMap<String, Object>>>() {}),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what was the reason for removing the ExpectedArtifact here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've committed this change in its own commit within the PR so it'd be documented: 27ed129
The test added in the commit would fail without this code change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@@ -228,14 +229,14 @@ public ArtifactResolver(

public void resolveArtifacts(@Nonnull Map pipeline) {
Map<String, Object> trigger = (Map<String, Object>) pipeline.get("trigger");

List<?> originalExpectedArtifacts =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you describe what problem you ran into here and why all the extra code was necessary to work around it - not sure I totally understand the justification just from looking at it

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was also committed in its own commit to help document the change: 688e1f9 It should explain the problem at the method level.
Feel free to read these issues/PRs to better grasp the overall context: https://docs.google.com/spreadsheets/d/112ul54xZGcN5pgKfd4hJlcstDGue-0LWghyNQ8lqBEc
A better fix would require a lot more time/effort, although probably not too much for someone with deep knowlege of the codebase.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@robzienert
Copy link
Member

gradle.properties will need to be updated to match the latest Keiko release.

@pdelagrave
Copy link
Author

Working on fixing some Jackson 2.10 incompatibilities that were forgotten, my bad for the delay.

Pierre Delagrave added 2 commits December 12, 2019 13:38
* jackson-module-kotlin changed the serialization behaviour for fields starting with 'is': FasterXML/jackson-module-kotlin#80
* jackson 2.10's objectMapper.convertValue() now converts everything even if the source is already from the target type. The fixed test was expected a value from a not fully converted Map.
@pdelagrave
Copy link
Author

Waiting on spinnaker/kork#442 and the associated keiko PR, then will commit gradle.properties updated with kork and keiko versions, which will be the final commit.

@robzienert
Copy link
Member

@cfieber cfieber added the ready to merge Approved and ready for merge label Dec 17, 2019
@mergify mergify bot merged commit df71ed9 into spinnaker:master Dec 17, 2019
@mergify mergify bot added the auto merged Merged automatically by a bot label Dec 17, 2019
louisjimenez added a commit to louisjimenez/orca that referenced this pull request Dec 20, 2019
louisjimenez added a commit to louisjimenez/orca that referenced this pull request Dec 20, 2019
@pdelagrave pdelagrave deleted the boot-2.2 branch January 6, 2020 20:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto merged Merged automatically by a bot ready to merge Approved and ready for merge target-release/1.18
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants