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(kubernetes): add support to configure control plane firewall #1310

Merged
merged 1 commit into from
Jan 29, 2025

Conversation

basert
Copy link
Contributor

@basert basert commented Jan 27, 2025

@basert basert force-pushed the kubernetes-cluster-firewall branch from 5168088 to 8e954c1 Compare January 28, 2025 10:12
@basert basert force-pushed the kubernetes-cluster-firewall branch from 8e954c1 to e4c76db Compare January 28, 2025 14:41
@loosla
Copy link
Contributor

loosla commented Jan 28, 2025

@basert Fabian, Thanks again for a great contribution 🚀
I left some suggestions that should allow acceptance tests to pass.
The rest looks great! Ready to approve when we fix the tests 🙂

@basert
Copy link
Contributor Author

basert commented Jan 29, 2025

@basert Fabian, Thanks again for a great contribution 🚀 I left some suggestions that should allow acceptance tests to pass. The rest looks great! Ready to approve when we fix the tests 🙂

Uhh, I don't see any of your suggestions :) Can I see the test results? I only have access to our company DO account and I dont wanna run the tests against that environment.

@loosla
Copy link
Contributor

loosla commented Jan 29, 2025

@basert Hey Fabio, Sorry, I forgot to complete the review, which caused the comments to remain in the pending stage. I appreciate you bringing this to my attention. You should now be able to see the suggestions.
Thanks for your help in improving the DO Terraform provider! 🐬
Here is the current acceptance tests output:

=== RUN   TestAccDigitalOceanKubernetesCluster_ControlPlaneFirewall
=== PAUSE TestAccDigitalOceanKubernetesCluster_ControlPlaneFirewall
=== CONT  TestAccDigitalOceanKubernetesCluster_ControlPlaneFirewall
panic: control_plane_firewall: '': source data must be an array or slice, got map

goroutine 332 [running]:
github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema.(*ResourceData).Set(0xc000b89400, {0x123b421, 0x16}, {0x10b6180, 0xc000cb2330})
	/home/terraform-provider-digitalocean/vendor/github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema/resource_data.go:230 +0x2ae
github.com/digitalocean/terraform-provider-digitalocean/digitalocean/kubernetes.digitaloceanKubernetesClusterRead(0xc000bfd188, 0xc000bd45a0, 0xc000b89400)
	/home/terraform-provider-digitalocean/digitalocean/kubernetes/resource_kubernetes_cluster.go:423 +0x827

@basert basert force-pushed the kubernetes-cluster-firewall branch from e4c76db to d793cd8 Compare January 29, 2025 14:18
@basert
Copy link
Contributor Author

basert commented Jan 29, 2025

@basert Hey Fabio, Sorry, I forgot to complete the review, which caused the comments to remain in the pending stage. I appreciate you bringing this to my attention. You should now be able to see the suggestions. Thanks for your help in improving the DO Terraform provider! 🐬 Here is the current acceptance tests output:

=== RUN   TestAccDigitalOceanKubernetesCluster_ControlPlaneFirewall
=== PAUSE TestAccDigitalOceanKubernetesCluster_ControlPlaneFirewall
=== CONT  TestAccDigitalOceanKubernetesCluster_ControlPlaneFirewall
panic: control_plane_firewall: '': source data must be an array or slice, got map

goroutine 332 [running]:
github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema.(*ResourceData).Set(0xc000b89400, {0x123b421, 0x16}, {0x10b6180, 0xc000cb2330})
	/home/terraform-provider-digitalocean/vendor/github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema/resource_data.go:230 +0x2ae
github.com/digitalocean/terraform-provider-digitalocean/digitalocean/kubernetes.digitaloceanKubernetesClusterRead(0xc000bfd188, 0xc000bd45a0, 0xc000b89400)
	/home/terraform-provider-digitalocean/digitalocean/kubernetes/resource_kubernetes_cluster.go:423 +0x827

Awesome, thanks for the review, I adjusted the code to your suggestions.

Copy link
Contributor

@loosla loosla left a comment

Choose a reason for hiding this comment

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

Looks awesome! Thanks for doing an amazing job! 🚀 🦈

@loosla loosla merged commit 98f5bda into digitalocean:main Jan 29, 2025
3 checks passed
@loosla
Copy link
Contributor

loosla commented Jan 29, 2025

@basert Your changes are in v2.48.1 v2.48.2 in case you need it for work.

@alexanderruch
Copy link

caused a bug: #1318

@loosla
Copy link
Contributor

loosla commented Jan 30, 2025

caused a bug: #1318

👍 Fixed in v2.48.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants