refactor(*): Remove credentialAccount from all AWS-derived cloud providers #4017
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
clareliguori
approved these changes
Sep 10, 2019
robzienert
force-pushed
the
rem-crufty-mc-crufterson
branch
from
4ff21cd
to
647f993
Compare
|
yolo |
|
Belated LGTM, I also don’t recall where credentialAccount originated. |
ezimanyi
added a commit
that referenced
this pull request
Sep 12, 2019
The change in #4017 broke deserialization of the account property. This is due to some confusing inheritence where KubernetesAtomicOperationDescription has a field 'account' that overrides the default method in the CredentialsNameable interface. Adding @JsonProperty("credentials") to the interface method broke deserialization. This can be fixed by adding an explicit @JsonProperty("account") to the field in KubernetesAtomicOperationDescription. As this is subtle, also add a test to validate this (and prevent it from breaking in the future). While it might be better to rename the account field, it's used all over the V1 provider in groovy code, so this change is much safer as it keeps the interface of the class to consumers the same. The test that is added passes before #4017, breaks after that change, and passes again with this change.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What could possibly go wrong with this? Really. What could possibly go wrong with this? I know we have duplication for
credentialsvsaccount, this is standardizing oncredentials, since like... I don't even know whycredentialAccountever existed to begin with.It looks to me like most of the code for descriptions expects
credentials, whereasaccountisn't really referenced directly anyhow? (Looking atAtomicOperationConverters for the AWS-derived cloud providers).