-
Notifications
You must be signed in to change notification settings - Fork 30
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
Fix wrong type of providers.aws.bakeryDefaults.baseImages[].virtualizationSettings
#175
Fix wrong type of providers.aws.bakeryDefaults.baseImages[].virtualizationSettings
#175
Conversation
…ionSettings` as repeated value Rosco manages `virtualizationSettings` for `aws` as a list of settings where an item contains regional VM settings, like follows. aws: bakeryDefaults: baseImages: - baseImage: id: ubuntu shortDescription: v12.04 detailedDescription: Ubuntu Precise Pangolin v12.04 packageType: deb templateFile: aws-ebs.json virtualizationSettings: - region: us-east-1 virtualizationType: hvm instanceType: t2.micro sourceAmi: ami-d4aed0bc sshUserName: ubuntu spotPrice: "0" spotPriceAutoProduct: Linux/UNIX (Amazon VPC) I have confirmed that testdata also contains wrong schema around `virtualizationSettings`. As long as I could confirm from Rosco implementation, it had `virtualizationSettings` was typed as `List<AWSVirtuzliationSettings>`, therefore, I do believe this is mistake while generating testdata for `aws` provider. static class AWSBakeryDefaults { String awsAccessKey String awsSecretKey String awsSubnetId String awsVpcId Boolean awsAssociatePublicIpAddress String templateFile BakeRequest.VmType defaultVirtualizationType List<AWSOperatingSystemVirtualizationSettings> baseImages = [] } static class AWSOperatingSystemVirtualizationSettings { BakeOptions.BaseImage baseImage List<AWSVirtualizationSettings> virtualizationSettings = [] } https://github.com/spinnaker/rosco/blob/version-0.25.0/rosco-core/src/main/groovy/com/netflix/spinnaker/rosco/providers/aws/config/RoscoAWSConfiguration.groovy#L65
Thanks for pointing that out, seems that the build is failing though |
Let me figure out the cause of failure 🙇 |
…es[].virtualizationSettings` as plural
Thank you for pointing things out 🙇 I have fixed the broken unit test, and now it's passing locally. I do think now it should work on CI too, however, it's not automatically running as I'm the first time contributor to this project. It's nice if someone can re-run CI job 🙏 |
The main build is passing now, but The build uses golangci-lint for that check, in case you want to run it manually in your machine. |
Thank you very much for guidance 🙇♂️ I should have checked carefully what have been done in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fixes!
Rosco manages
virtualizationSettings
foraws
as a list of settings where an item contains regional VM settings, like follows.https://github.com/spinnaker/rosco/blob/version-0.25.0/halconfig/images.yml#L4-L122
However, so far Kleat wasn't allowing to have list value at
virtualizationSettings
because the field wasn't configured forrepeated
. At very least this prevented me to configure Rosco to bake AMIs, and needed to patch generatedclouddriver.yml
androsco.yml
to make those services working.It seems that
testdata
also contains wrong schema aroundvirtualizationSettings
. As long as I could confirm from Rosco implementation, it hadvirtualizationSettings
was typed asList<AWSVirtuzliationSettings>
, therefore, I do believe this is a mistake made while generating testdata foraws
provider.https://github.com/spinnaker/rosco/blob/version-0.25.0/rosco-core/src/main/groovy/com/netflix/spinnaker/rosco/providers/aws/config/RoscoAWSConfiguration.groovy#L65