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.

Sign up for GitHub

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

Jump to bottom

Fix serialization of pinned and vetoed fields #1377

Merged
merged 1 commit into from
Jul 14, 2020
Merged

Fix serialization of pinned and vetoed fields #1377

merged 1 commit into from
Jul 14, 2020

Conversation

robfletcher
Copy link
Contributor

Fixes serialization of fields in artifact/environment summary. Instead of boolean fields and objects now there are just the objects.

@robfletcher robfletcher force-pushed the fix-pinned-vetoed-serialization branch from 71b6b15 to e657d99 Compare July 14, 2020 03:25
gal-yardeni
gal-yardeni reviewed Jul 14, 2020
View reviewed changes
...-core-test/src/main/kotlin/com/netflix/spinnaker/keel/persistence/ArtifactRepositoryTests.kt
@@ -610,7 +610,6 @@ abstract class ArtifactRepositoryTests<T : ArtifactRepository> : JUnit5Minutests
)
expect {
that(envArtifactSummary).isNotNull()
that(envArtifactSummary?.isPinned).isTrue()
Copy link
Contributor

Choose a reason for hiding this comment

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

In order to keep the same functionality, should we add here (and below, if I understand the change correctly):
that(envArtifactSummary?.pinned).isNotNull() ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The next line does an isEqualTo on the same field so testing it's not null explicitly doesn't really add anything.

@robfletcher robfletcher force-pushed the fix-pinned-vetoed-serialization branch from e657d99 to 08371c3 Compare July 14, 2020 15:38
lorin
lorin approved these changes Jul 14, 2020
View reviewed changes
Copy link
Contributor

@lorin lorin left a comment

Choose a reason for hiding this comment

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

This change looks fine to me, but it looks like it's just removing some unused fields from the API payload. I don't understand how the presence of those fields could have broken the UI. @erikmunson, can you provide some insight here?

@robfletcher
Copy link
Contributor Author

There was a change made in a recent version of Jackson so that those field definitions conflict. It started serializing pinned as isPinned and because there was already an isPinned property, one overrode the other.

The change in behavior seems to have be prompted by this issue: FasterXML/jackson-module-kotlin#80 unfortunately the "fix" was a breaking change. The only way I could successfully get both properties to serialize was to annotate each twice with @JsonProperty (once on the property itself, once on the getter, i.e. using @get:JsonProperty syntax). Anything less that 2 annotations on each property either results in one property overriding the other in the serialized output, or Jackson throwing an exception because it detects the ambiguity.

@lorin
Copy link
Contributor

lorin commented Jul 14, 2020

The only way I could successfully get both properties to serialize was to annotate each twice with @JsonProperty (once on the property itself, once on the getter, i.e. using @get:JsonProperty syntax). Anything less that 2 annotations on each property either results in one property overriding the other in the serialized output, or Jackson throwing an exception because it detects the ambiguity.

🤮

@robfletcher
Copy link
Contributor Author

@erikmunson does this look like the format that will un-fuck the environments view?

@erikmunson
Copy link
Member

Yep looks right to me - thanks for getting it resolved! I'm hopeful there is a way to prevent backend folks from having to deal with this property overriding business in the future 🤞

@robfletcher
Copy link
Contributor Author

Yep looks right to me - thanks for getting it resolved! I'm hopeful there is a way to prevent backend folks from having to deal with this property overriding business in the future 🤞

Yeah, I think I'm going to raise an issue on the jackson-module-kotlin repo

@robfletcher robfletcher added the ready to merge Approved and ready for merge label Jul 14, 2020
@mergify mergify bot merged commit f1ebd02 into spinnaker:master Jul 14, 2020
@mergify mergify bot added the auto merged label Jul 14, 2020
@robfletcher robfletcher deleted the fix-pinned-vetoed-serialization branch July 14, 2020 19:37
luispollo pushed a commit to luispollo/keel that referenced this pull request May 27, 2022
@ajordens @luispollo
Pull request spinnaker#1377: feat(deployment-locations): Initial Depl…
5f467a1 
…oyment Locations plugin

Merge in SPKR/keel-nflx from ~AJORDENS/keel-nflx:feature/deployment-locations to main

Squashed commit of the following:

commit fe0da5a04c395184ffebe378801645d5464280b9
Author: Adam Jordens <[email protected]>
Date:   Thu May 12 18:00:10 2022 -0700

    feat(workload-locations): Initial Workload Locations plugin
    _(not really a plugin but it does only depend on `keel-api`)_

    This PR introduces a new `SubmittedEnvironmentPreProcessor` interface
    allowing for a submitted environment to be manipulated prior to
    deserialization.

    The simplistic current implementation does support the basic flow of
    going from workload type to location. It will be extended to allow
    non-standand vpc/subnets to be chosen.

    Resources are preprocessed during deserialization and locations are
    set where not already specified.

    See test cases for usage.

    Things we may want to consider:
    * Support for multiple accounts in a `SubnetAwareLocations` (or `locations` being a `List<SubnetAwareLocations>`)
    * Support for multiple cloud providers (eg. a workload type that supports both aws and titus)
    * ...

    To disable, set the following FP to false:
    `keel.plugins.workload-locations.enabled`

commit 4b664ddcefb274cba6cae7c45afc53ae9cf40f43
Author: Adam Jordens <[email protected]>
Date:   Thu May 12 17:48:21 2022 -0700

    feat(deployment-locations): Static metadata for use when translating from workload types

    - `spinnaker_subnets` and `spinnaker_vpcs` come directly from Spinnaker APIs.
    - `spinnaker_accounts` are handcrafted but based on `clouddriver.yml`.
    - `location_sets` and `workload_types` are new concepts supporting the abstraction.

    While static metadata makes for quick experimentation, it's expected that this data will be moved to
    S3 or Infra API eventually (or perhaps the entire translation will be moved to an external service).

(cherry picked from commit e728461ae17c89547aaffd243615f622378f2f3d)
osoriano pushed a commit to osoriano/keel that referenced this pull request Sep 2, 2023
@ajordens @osoriano
Pull request spinnaker#1377: feat(deployment-locations): Initial Depl…
cca1b72 
…oyment Locations plugin

Merge in SPKR/keel-nflx from ~AJORDENS/keel-nflx:feature/deployment-locations to main

Squashed commit of the following:

commit fe0da5a04c395184ffebe378801645d5464280b9
Author: Adam Jordens <[email protected]>
Date:   Thu May 12 18:00:10 2022 -0700

    feat(workload-locations): Initial Workload Locations plugin
    _(not really a plugin but it does only depend on `keel-api`)_

    This PR introduces a new `SubmittedEnvironmentPreProcessor` interface
    allowing for a submitted environment to be manipulated prior to
    deserialization.

    The simplistic current implementation does support the basic flow of
    going from workload type to location. It will be extended to allow
    non-standand vpc/subnets to be chosen.

    Resources are preprocessed during deserialization and locations are
    set where not already specified.

    See test cases for usage.

    Things we may want to consider:
    * Support for multiple accounts in a `SubnetAwareLocations` (or `locations` being a `List<SubnetAwareLocations>`)
    * Support for multiple cloud providers (eg. a workload type that supports both aws and titus)
    * ...

    To disable, set the following FP to false:
    `keel.plugins.workload-locations.enabled`

commit 4b664ddcefb274cba6cae7c45afc53ae9cf40f43
Author: Adam Jordens <[email protected]>
Date:   Thu May 12 17:48:21 2022 -0700

    feat(deployment-locations): Static metadata for use when translating from workload types

    - `spinnaker_subnets` and `spinnaker_vpcs` come directly from Spinnaker APIs.
    - `spinnaker_accounts` are handcrafted but based on `clouddriver.yml`.
    - `location_sets` and `workload_types` are new concepts supporting the abstraction.

    While static metadata makes for quick experimentation, it's expected that this data will be moved to
    S3 or Infra API eventually (or perhaps the entire translation will be moved to an external service).

(cherry picked from commit e728461ae17c89547aaffd243615f622378f2f3d)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gal-yardeni gal-yardeni gal-yardeni left review comments

@lorin lorin lorin approved these changes

@emjburns emjburns emjburns approved these changes

@erikmunson erikmunson Awaiting requested review from erikmunson

@luispollo luispollo Awaiting requested review from luispollo

Assignees
No one assigned
Labels
auto merged ready to merge Approved and ready for merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@robfletcher @lorin @erikmunson @emjburns @gal-yardeni