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

feat(TPG>=5.40.0)!: Add support for RayOperator Addon #2032

Merged
merged 7 commits into from
Aug 10, 2024
Merged

feat(TPG>=5.40.0)!: Add support for RayOperator Addon #2032

merged 7 commits into from
Aug 10, 2024

Conversation

genlu2011
Copy link
Contributor

Support for feature was added in 5.40.0 of terraform-provider-google: https://github.com/hashicorp/terraform-provider-google/releases/tag/v5.40.0

container: added field ray_operator_config for resource_container_cluster (hashicorp/terraform-provider-google#18825)

@genlu2011
feat(TPG>=5.25.0)!: Add support for RayOperator Addon
4019706 
Change-Id: Ic8d3e1bece5088c569608dda2b05c58bcad59636
@genlu2011 genlu2011 requested review from ericyz, gtsorbo and a team as code owners August 7, 2024 22:12
@apeabody apeabody self-assigned this Aug 7, 2024
@apeabody
Copy link
Collaborator

apeabody commented Aug 7, 2024

/gcbrun

apeabody
apeabody requested changes Aug 8, 2024
View reviewed changes
Copy link
Collaborator

@apeabody apeabody left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @genlu2011!

It would be best to exercise this new capability in an appropriate existing samples, so it has test coverage: https://github.com/terraform-google-modules/terraform-google-kubernetes-engine/tree/master/examples

Even better would to be then validate the cluster state (for Ray Operator Addon) in the respective test verify: https://github.com/terraform-google-modules/terraform-google-kubernetes-engine/tree/master/test/integration

@genlu2011
fix: add tests
1a0907b 
Change-Id: Id54e003f5b52675be3076082cc09931f6659b86c
@apeabody
Copy link
Collaborator

apeabody commented Aug 8, 2024

/gcbrun

@genlu2011
fix: typo
7c45c53 
Change-Id: I47d609a3121aee2badc4445b31189114278e5ba2
@apeabody
Copy link
Collaborator

apeabody commented Aug 8, 2024

/gcbrun

@genlu2011
Run lint
9282a65 
Change-Id: Idcadb513f5dba1f97581319def202cc68b5c40de
@apeabody
Copy link
Collaborator

apeabody commented Aug 8, 2024

/gcbrun

1 similar comment
@apeabody
Copy link
Collaborator

apeabody commented Aug 8, 2024

/gcbrun

@genlu2011
Copy link
Contributor Author

Thanks for the contribution @genlu2011!

It would be best to exercise this new capability in an appropriate existing samples, so it has test coverage: https://github.com/terraform-google-modules/terraform-google-kubernetes-engine/tree/master/examples

Even better would to be then validate the cluster state (for Ray Operator Addon) in the respective test verify: https://github.com/terraform-google-modules/terraform-google-kubernetes-engine/tree/master/test/integration

Added test.

@apeabody
Copy link
Collaborator

apeabody commented Aug 8, 2024

/gcbrun

@apeabody
Copy link
Collaborator

apeabody commented Aug 8, 2024

Going to re-trigger

    private_zonal_with_networking_test.go:79: 
        	Error Trace:	/workspace/test/integration/private_zonal_with_networking/private_zonal_with_networking_test.go:79
        	            				/builder/home/go/pkg/mod/github.com/!google!cloud!platform/cloud-foundation-toolkit/infra/[email protected]/pkg/tft/terraform.go:638
        	            				/builder/home/go/pkg/mod/github.com/!google!cloud!platform/cloud-foundation-toolkit/infra/[email protected]/pkg/tft/terraform.go:670
        	            				/builder/home/go/pkg/mod/github.com/!google!cloud!platform/cloud-foundation-toolkit/infra/[email protected]/pkg/utils/stages.go:31
        	            				/builder/home/go/pkg/mod/github.com/!google!cloud!platform/cloud-foundation-toolkit/infra/[email protected]/pkg/tft/terraform.go:670
        	Error:      	Should be false
        	Test:       	TestPrivateZonalWithNetworking
        	Messages:   	does not have autoscaling enabled

@apeabody
Copy link
Collaborator

apeabody commented Aug 9, 2024

/gcbrun

@apeabody
Copy link
Collaborator

apeabody commented Aug 9, 2024

looks like a spacing difference in the test data:

Step #94 - "verify simple-autopilot-public-local":         	Error Trace:	/builder/home/go/pkg/mod/github.com/!google!cloud!platform/cloud-foundation-toolkit/infra/[email protected]/pkg/golden/golden.go:157
Step #94 - "verify simple-autopilot-public-local":         	            				/workspace/test/integration/simple_autopilot_public/simple_autopiliot_public_test.go:59
Step #94 - "verify simple-autopilot-public-local":         	            				/builder/home/go/pkg/mod/github.com/!google!cloud!platform/cloud-foundation-toolkit/infra/[email protected]/pkg/tft/terraform.go:638
Step #94 - "verify simple-autopilot-public-local":         	            				/builder/home/go/pkg/mod/github.com/!google!cloud!platform/cloud-foundation-toolkit/infra/[email protected]/pkg/tft/terraform.go:670
Step #94 - "verify simple-autopilot-public-local":         	            				/builder/home/go/pkg/mod/github.com/!google!cloud!platform/cloud-foundation-toolkit/infra/[email protected]/pkg/utils/stages.go:31
Step #94 - "verify simple-autopilot-public-local":         	            				/builder/home/go/pkg/mod/github.com/!google!cloud!platform/cloud-foundation-toolkit/infra/[email protected]/pkg/tft/terraform.go:670
Step #94 - "verify simple-autopilot-public-local":         	Error:      	Not equal: 
Step #94 - "verify simple-autopilot-public-local":         	            	expected: "{\n        \"enabled\": true,\n        \"rayClusterLoggingConfig\": {\n          \"enabled\": true\n        },\n        \"rayClusterMonitoringConfig\": {\n          \"enabled\": true\n        }\n      }"
Step #94 - "verify simple-autopilot-public-local":         	            	actual  : "{\n      \"enabled\": true,\n      \"rayClusterLoggingConfig\": {\n        \"enabled\": true\n      },\n      \"rayClusterMonitoringConfig\": {\n        \"enabled\": true\n      }\n    }"
Step #94 - "verify simple-autopilot-public-local":         	            	
Step #94 - "verify simple-autopilot-public-local":         	            	Diff:
Step #94 - "verify simple-autopilot-public-local":         	            	--- Expected
Step #94 - "verify simple-autopilot-public-local":         	            	+++ Actual
Step #94 - "verify simple-autopilot-public-local":         	            	@@ -1,9 +1,9 @@
Step #94 - "verify simple-autopilot-public-local":         	            	 {
Step #94 - "verify simple-autopilot-public-local":         	            	-        "enabled": true,
Step #94 - "verify simple-autopilot-public-local":         	            	-        "rayClusterLoggingConfig": {
Step #94 - "verify simple-autopilot-public-local":         	            	-          "enabled": true
Step #94 - "verify simple-autopilot-public-local":         	            	-        },
Step #94 - "verify simple-autopilot-public-local":         	            	-        "rayClusterMonitoringConfig": {
Step #94 - "verify simple-autopilot-public-local":         	            	-          "enabled": true
Step #94 - "verify simple-autopilot-public-local":         	            	-        }
Step #94 - "verify simple-autopilot-public-local":         	            	+      "enabled": true,
Step #94 - "verify simple-autopilot-public-local":         	            	+      "rayClusterLoggingConfig": {
Step #94 - "verify simple-autopilot-public-local":         	            	+        "enabled": true
Step #94 - "verify simple-autopilot-public-local":         	            	+      },
Step #94 - "verify simple-autopilot-public-local":         	            	+      "rayClusterMonitoringConfig": {
Step #94 - "verify simple-autopilot-public-local":         	            	+        "enabled": true
Step #94 - "verify simple-autopilot-public-local":         	            	       }
Step #94 - "verify simple-autopilot-public-local":         	            	+    }
Step #94 - "verify simple-autopilot-public-local":         	Test:       	TestSimpleAutopilotPublic
Step #94 - "verify simple-autopilot-public-local":         	Messages:   	expected addonsConfig.rayOperatorConfig to match fixture {
Step #94 - "verify simple-autopilot-public-local":         	            	        "enabled": true,
Step #94 - "verify simple-autopilot-public-local":         	            	        "rayClusterLoggingConfig": {
Step #94 - "verify simple-autopilot-public-local":         	            	          "enabled": true
Step #94 - "verify simple-autopilot-public-local":         	            	        },
Step #94 - "verify simple-autopilot-public-local":         	            	        "rayClusterMonitoringConfig": {
Step #94 - "verify simple-autopilot-public-local":         	            	          "enabled": true
Step #94 - "verify simple-autopilot-public-local":         	            	        }
Step #94 - "verify simple-autopilot-public-local":         	            	      }

@genlu2011
fix: compare field instead of json
1e81a78 
Change-Id: I69818a30061486b905d4ef63d13288d57c80a784
@genlu2011
Copy link
Contributor Author

looks like a spacing difference in the test data:

Step #94 - "verify simple-autopilot-public-local":         	Error Trace:	/builder/home/go/pkg/mod/github.com/!google!cloud!platform/cloud-foundation-toolkit/infra/[email protected]/pkg/golden/golden.go:157
Step #94 - "verify simple-autopilot-public-local":         	            				/workspace/test/integration/simple_autopilot_public/simple_autopiliot_public_test.go:59
Step #94 - "verify simple-autopilot-public-local":         	            				/builder/home/go/pkg/mod/github.com/!google!cloud!platform/cloud-foundation-toolkit/infra/[email protected]/pkg/tft/terraform.go:638
Step #94 - "verify simple-autopilot-public-local":         	            				/builder/home/go/pkg/mod/github.com/!google!cloud!platform/cloud-foundation-toolkit/infra/[email protected]/pkg/tft/terraform.go:670
Step #94 - "verify simple-autopilot-public-local":         	            				/builder/home/go/pkg/mod/github.com/!google!cloud!platform/cloud-foundation-toolkit/infra/[email protected]/pkg/utils/stages.go:31
Step #94 - "verify simple-autopilot-public-local":         	            				/builder/home/go/pkg/mod/github.com/!google!cloud!platform/cloud-foundation-toolkit/infra/[email protected]/pkg/tft/terraform.go:670
Step #94 - "verify simple-autopilot-public-local":         	Error:      	Not equal: 
Step #94 - "verify simple-autopilot-public-local":         	            	expected: "{\n        \"enabled\": true,\n        \"rayClusterLoggingConfig\": {\n          \"enabled\": true\n        },\n        \"rayClusterMonitoringConfig\": {\n          \"enabled\": true\n        }\n      }"
Step #94 - "verify simple-autopilot-public-local":         	            	actual  : "{\n      \"enabled\": true,\n      \"rayClusterLoggingConfig\": {\n        \"enabled\": true\n      },\n      \"rayClusterMonitoringConfig\": {\n        \"enabled\": true\n      }\n    }"
Step #94 - "verify simple-autopilot-public-local":         	            	
Step #94 - "verify simple-autopilot-public-local":         	            	Diff:
Step #94 - "verify simple-autopilot-public-local":         	            	--- Expected
Step #94 - "verify simple-autopilot-public-local":         	            	+++ Actual
Step #94 - "verify simple-autopilot-public-local":         	            	@@ -1,9 +1,9 @@
Step #94 - "verify simple-autopilot-public-local":         	            	 {
Step #94 - "verify simple-autopilot-public-local":         	            	-        "enabled": true,
Step #94 - "verify simple-autopilot-public-local":         	            	-        "rayClusterLoggingConfig": {
Step #94 - "verify simple-autopilot-public-local":         	            	-          "enabled": true
Step #94 - "verify simple-autopilot-public-local":         	            	-        },
Step #94 - "verify simple-autopilot-public-local":         	            	-        "rayClusterMonitoringConfig": {
Step #94 - "verify simple-autopilot-public-local":         	            	-          "enabled": true
Step #94 - "verify simple-autopilot-public-local":         	            	-        }
Step #94 - "verify simple-autopilot-public-local":         	            	+      "enabled": true,
Step #94 - "verify simple-autopilot-public-local":         	            	+      "rayClusterLoggingConfig": {
Step #94 - "verify simple-autopilot-public-local":         	            	+        "enabled": true
Step #94 - "verify simple-autopilot-public-local":         	            	+      },
Step #94 - "verify simple-autopilot-public-local":         	            	+      "rayClusterMonitoringConfig": {
Step #94 - "verify simple-autopilot-public-local":         	            	+        "enabled": true
Step #94 - "verify simple-autopilot-public-local":         	            	       }
Step #94 - "verify simple-autopilot-public-local":         	            	+    }
Step #94 - "verify simple-autopilot-public-local":         	Test:       	TestSimpleAutopilotPublic
Step #94 - "verify simple-autopilot-public-local":         	Messages:   	expected addonsConfig.rayOperatorConfig to match fixture {
Step #94 - "verify simple-autopilot-public-local":         	            	        "enabled": true,
Step #94 - "verify simple-autopilot-public-local":         	            	        "rayClusterLoggingConfig": {
Step #94 - "verify simple-autopilot-public-local":         	            	          "enabled": true
Step #94 - "verify simple-autopilot-public-local":         	            	        },
Step #94 - "verify simple-autopilot-public-local":         	            	        "rayClusterMonitoringConfig": {
Step #94 - "verify simple-autopilot-public-local":         	            	          "enabled": true
Step #94 - "verify simple-autopilot-public-local":         	            	        }
Step #94 - "verify simple-autopilot-public-local":         	            	      }

Yeah, I change the test to compare fields.

@apeabody
Copy link
Collaborator

apeabody commented Aug 9, 2024

/gcbrun

@apeabody apeabody merged commit c046af1 into terraform-google-modules:master Aug 10, 2024
4 checks passed
@release-please release-please bot mentioned this pull request Aug 10, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@apeabody apeabody apeabody approved these changes

@ericyz ericyz Awaiting requested review from ericyz ericyz is a code owner

@gtsorbo gtsorbo Awaiting requested review from gtsorbo

Assignees

@apeabody apeabody

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@genlu2011 @apeabody