-
Notifications
You must be signed in to change notification settings - Fork 1k
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(gce): support resourceManagerTags in instance template #6282
Conversation
Please delete the pull request instructions from the body of your pull request message. The instructions start with the line:
You can reopen your pull request when this has been addressed. |
Are there any meaningful tests that can be added for this? |
I am still working on adding test for it but it is challenging because the BasicGoogleDeployHandler class is really hard to test :( |
* chore(dependencies): Autobump korkVersion * refactor(dependency): replace groovy coordinates and code adjustment to accomodate upgrade of groovy 4.x - Replacing the groovy coordinates from `org.codehaus.groovy` to `org.apache.groovy` supported by groovy 4.x and above versions. - Unpinning `io.rest-assured` to get in sync with kork. - Removed unused import from class `com.netflix.spinnaker.clouddriver.oracle.deploy.op.DestroyOracleServerGroupAtomicOperationSpec.groovy`. - While upgrading groovy 4.0.15: Encounter below error during build process of clouddriver-appengine module and similar errors in clouddriver-aws, clouddriver-azure, clouddriver-core, clouddriver-google and clouddriver-oracle modules : ``` /clouddriver/clouddriver-appengine/build/tmp/compileGroovy/groovy-java-stubs/com/netflix/spinnaker/clouddriver/appengine/model/AppengineServerGroup.java:3: error: AppengineServerGroup is not abstract and does not override abstract method isDisabled() in ServerGroup @groovy.transform.CompileStatic() @groovy.transform.EqualsAndHashCode(includes={"name","account"}) public class AppengineServerGroup ^ 1 error startup failed: Compilation failed; see the compiler error output for details. 1 error > Task :clouddriver-appengine:compileGroovy FAILED ``` To fix this issue added `isDisabled()` overriden method. Encounter below error during test execution of clouddriver-aws module and similar errors in clouddriver-google module: ``` No such property: SUBNET_ID_OVERRIDE_TAG for class: com.netflix.spinnaker.clouddriver.aws.deploy.handlers.BasicAmazonDeployHandlerUnitSpec$1 groovy.lang.MissingPropertyException: No such property: SUBNET_ID_OVERRIDE_TAG for class: com.netflix.spinnaker.clouddriver.aws.deploy.handlers.BasicAmazonDeployHandlerUnitSpec$1 at app//groovy.lang.GroovyObject.getProperty(GroovyObject.java:50) at app//groovy.lang.Closure.getPropertyTryThese(Closure.java:325) at app//groovy.lang.Closure.getPropertyOwnerFirst(Closure.java:319) at app//groovy.lang.Closure.getProperty(Closure.java:309) at com.netflix.spinnaker.clouddriver.aws.deploy.handlers.BasicAmazonDeployHandler.copySourceAttributes_closure6(BasicAmazonDeployHandler.groovy:384) at app//com.netflix.spinnaker.clouddriver.aws.deploy.handlers.BasicAmazonDeployHandler.copySourceAttributes(BasicAmazonDeployHandler.groovy:384) at com.netflix.spinnaker.clouddriver.aws.deploy.handlers.BasicAmazonDeployHandlerUnitSpec.should copy subnet ids from source when available and not explicitly specified(BasicAmazonDeployHandlerUnitSpec.groovy:590) ``` In order to fix this issue, replace the static final variable with qualified name using classname. It may be related to this [issue](https://issues.apache.org/jira/browse/GROOVY-9386). -------- Encounter below error during test execution of `AmazonCloudMetricProviderSpec.groovy` class in clouddriver-aws module: ``` No such property: args for class: com.netflix.spinnaker.clouddriver.aws.model.AmazonMetricDescriptor groovy.lang.MissingPropertyException: No such property: args for class: com.netflix.spinnaker.clouddriver.aws.model.AmazonMetricDescriptor at app//com.netflix.spinnaker.clouddriver.aws.model.AmazonMetricDescriptor.from(AmazonMetricDescriptor.groovy:41) at app//com.netflix.spinnaker.clouddriver.aws.provider.view.AmazonCloudMetricProvider.getMetricDescriptor(AmazonCloudMetricProvider.groovy:66) at com.netflix.spinnaker.clouddriver.aws.provider.view.AmazonCloudMetricProviderSpec.getMetrics returns a metric when one found(AmazonCloudMetricProviderSpec.groovy:74) ``` To fix this issue, created an explicit constructor and removed `@Immutable`. ---------- Encounter below error during test execution in clouddriver-core module: ``` No such property: completed for class: com.netflix.spinnaker.clouddriver.data.task.DefaultTaskStatus groovy.lang.MissingPropertyException: No such property: completed for class: com.netflix.spinnaker.clouddriver.data.task.DefaultTaskStatus at app//org.codehaus.groovy.runtime.ScriptBytecodeAdapter.unwrap(ScriptBytecodeAdapter.java:67) at app//org.codehaus.groovy.vmplugin.v8.IndyGuardsFiltersAndSignatures.unwrap(IndyGuardsFiltersAndSignatures.java:163) at app//org.codehaus.groovy.vmplugin.v8.IndyInterface.fromCache(IndyInterface.java:321) at app//com.netflix.spinnaker.clouddriver.data.task.InMemoryTaskRepository.list(InMemoryTaskRepository.groovy:54) at app//com.netflix.spinnaker.clouddriver.core.test.TaskRepositoryTck.testListRunningTasks(TaskRepositoryTck.java:107) ``` To fix this issue, replaced the fields with its getter method. ---------- Encounter below error during test execution of `ProjectClustersServiceSpec.groovy` class in clouddriver-core module, and similar error in clouddriver-titus module: ``` > Task :clouddriver-core:compileTestGroovy FAILED startup failed: /clouddriver/clouddriver-core/src/test/groovy/com/netflix/spinnaker/clouddriver/core/ProjectClustersServiceSpec.groovy: 495: The return type of boolean isDisabled() in com.netflix.spinnaker.clouddriver.core.ProjectClustersServiceSpec$TestServerGroup is incompatible with java.lang.Boolean in com.netflix.spinnaker.clouddriver.model.ServerGroup . At [495:5] @ line 495, column 5. boolean disabled ^ 1 error ``` To fix this issue, replaced `boolean` primitive type to `Boolean` type. ---------- Encounter below error during compilation of clouddriver-google module: ``` /clouddriver/clouddriver-google/build/tmp/compileGroovy/groovy-java-stubs/com/netflix/spinnaker/clouddriver/google/deploy/exception/GoogleResourceNotFoundException.java:5: error: no suitable constructor found for GoogleOperationException(no arguments) @groovy.transform.InheritConstructors() public class GoogleResourceNotFoundException ^ constructor GoogleOperationException.GoogleOperationException(String) is not applicable (actual and formal argument lists differ in length) constructor GoogleOperationException.GoogleOperationException(String,Throwable) is not applicable (actual and formal argument lists differ in length) /clouddriver/clouddriver-google/build/tmp/compileGroovy/groovy-java-stubs/com/netflix/spinnaker/clouddriver/google/deploy/exception/GoogleOperationTimedOutException.java:5: error: no suitable constructor found for GoogleOperationException(no arguments) @groovy.transform.InheritConstructors() public class GoogleOperationTimedOutException ^ constructor GoogleOperationException.GoogleOperationException(String) is not applicable (actual and formal argument lists differ in length) constructor GoogleOperationException.GoogleOperationException(String,Throwable) is not applicable (actual and formal argument lists differ in length) Note: Some input files use or override a deprecated API. Note: Recompile with -Xlint:deprecation for details. Note: Some input files use unchecked or unsafe operations. Note: Recompile with -Xlint:unchecked for details. 3 errors startup failed: Compilation failed; see the compiler error output for details. 1 error > Task :clouddriver-google:compileGroovy FAILED ``` To fix this issue, added a default constructor to `GoogleOperationException.groovy` class. ---------- Encounter below error during test execution in clouddriver-azure module: ``` Cannot invoke method isWarnEnabled() on null object java.lang.NullPointerException: Cannot invoke method isWarnEnabled() on null object at com.netflix.spinnaker.clouddriver.azure.resources.servergroup.ops.preprocessors.RegionsToRegionDescriptionPreProcessor.process_closure1(RegionsToRegionDescriptionPreProcessor.groovy:45) at groovy.lang.Closure.call(Closure.java:433) at groovy.lang.Closure.call(Closure.java:422) at com.netflix.spinnaker.clouddriver.azure.resources.servergroup.ops.preprocessors.RegionsToRegionDescriptionPreProcessor.process(RegionsToRegionDescriptionPreProcessor.groovy:40) at com.netflix.spinnaker.clouddriver.azure.resources.servergroup.ops.preprocessors.RegionsToRegionDescriptionPreProcessorSpec.should convert legacy descriptions(RegionsToRegionDescriptionPreProcessorSpec.groovy:33) ``` To fix this issue, explicitly defined a method to return `log` object inside `with{}` closure. It may be related to this [issue](https://issues.apache.org/jira/browse/GROOVY-8820) --------- Co-authored-by: root <root@84926203b695> Co-authored-by: j-sandy <30489233+j-sandy@users.noreply.github.com>
Co-authored-by: root <root@15a60bc3bd71>
# Conflicts: # clouddriver-google/clouddriver-google.gradle
@jasonmcintosh @dbyron-sf @christosarvanitis I tried to add a test for the resourceManagerTags field I added to GCE but I am not able to test the |
@edgarulg I'm all for the refactor happening in a separate PR, especially if it can happen before this one. |
Hi @dbyron-sf. Normally I would be in full-throated agreement. This PR is a minimal change (adding one new field). Refactoring here would introduce more risk and have a greater chance of creating regressions. We are happy to take a crack at refactoring this to make it more testable soon after. |
I am closing this PR because a lot of changes were added from master and it isn't clear what changes belongs to the |
Support
resourceManagerTags
in GCE. See: https://cloud.google.com/compute/docs/reference/rest/v1/instanceTemplates/insertAdds
resourceManagerTags
in the description operation and add it in the instanceTemplateProperties.Issue related: spinnaker/spinnaker#6931