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

feat(serialization): Use friendly names for artifact types #560

Merged
merged 3 commits into from
Oct 30, 2019

Conversation

luispollo
Copy link
Contributor

@luispollo luispollo commented Oct 29, 2019

Addresses #557 by introducing "friendly names" for artifact types (i.e. lower-case names as opposed to the default enum value names). Given that all current friendly names match the upper-case enum names, this could probably be even simpler, but when I started we had a conversion between "docker" and DOCKER_TYPE and so I figured it's OK to have a place to add that kind of logic if we ever have other types that don't match the friendly name. The @JsonCreator factory method also allows for backwards-compatibility with existing specs still using the upper-as names.

DEB
enum class ArtifactType(@JsonValue val friendlyName: String) {
DEB("deb"),
DOCKER_IMAGE("docker");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will conflict with @emjburns' change in #556. I'll wait for her to merge and then resolve the conflict.

@luispollo
Copy link
Contributor Author

@emjburns @erikmunson Please review.

@erikmunson
Copy link
Member

erikmunson commented Oct 29, 2019

this is great! thanks Luis. FWIW, it appears Emily switched the enum value to DOCKER after Rob and I bugged her about it, so you may be able to omit some of the mapping stuff here (maybe? I don't know these things): https://github.com/spinnaker/keel/pull/556/files#diff-2cd4b503a4f236b6c490d3fcccb3481dR9

@luispollo
Copy link
Contributor Author

luispollo commented Oct 29, 2019

Hey @erikmunson, yes, I noticed that after I made my change. I'll remove the line to convert from "docker" to DOCKER_IMAGE as soon as @emjburns merges her PR.

@luispollo
Copy link
Contributor Author

@emjburns @erikmunson This is now rebased after Emily's PR was merged. @robfletcher If you want to take a look as well, please do!

@emjburns emjburns requested review from emjburns and robfletcher and removed request for emjburns October 30, 2019 17:17
Copy link
Contributor

@emjburns emjburns left a comment

Choose a reason for hiding this comment

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

😆 looks good! This will work if you have either deb or DEB in the yaml, right?

@erikmunson
Copy link
Member

Oh related question to Emily's — what will the export resource endpoint emit in its YAML for delivery artifacts after this change? Ideally it would prefer the lower case form.

@luispollo
Copy link
Contributor Author

luispollo commented Oct 30, 2019

😆 looks good! This will work if you have either deb or DEB in the yaml, right?

@emjburns Right, it will work with both formats. I added a test for that as well.

Oh related question to Emily's — what will the export resource endpoint emit in its YAML for delivery artifacts after this change? Ideally it would prefer the lower case form.

@erikmunson Yes, JSON output will be using the friendly format. For example:

---
metadata:
  serviceAccount: "keel@spinnaker.io"
apiVersion: "ec2.spinnaker.netflix.com/v1"
kind: "cluster"
spec:
  moniker:
    app: "keeldemo"
    stack: "main"
  imageProvider:
    deliveryArtifact:
      name: "keeldemo"
      type: "deb"
  locations:
    account: "test"
    subnet: "internal (vpc0)"
    vpc: "vpc0"
    regions:
    - name: "us-west-2"
  launchConfiguration:
    instanceType: "m5.large"
    ebsOptimized: true
    iamRole: "keeldemoInstanceProfile"
    keyPair: "nf-test-keypair-a"
    instanceMonitoring: false
  capacity:
    min: 1
    max: 1
    desired: 1
  dependencies:
    loadBalancerNames:
    - "keeldemo-main-clb"
    securityGroupNames:
    - "keeldemo"
    - "nf-datacenter"
    - "nf-infrastructure"

@luispollo
Copy link
Contributor Author

@emjburns If you're OK with it, could you please add the ready to merge label?

@emjburns emjburns added the ready to merge Approved and ready for merge label Oct 30, 2019
@mergify mergify bot merged commit 076e1bc into spinnaker:master Oct 30, 2019
@mergify mergify bot added the auto merged label Oct 30, 2019
@luispollo luispollo deleted the non-screaming-artifact-types branch November 4, 2019 17:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto merged ready to merge Approved and ready for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants