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

Add kubernetes_ingress_v1 timeouts #1936

Merged
merged 6 commits into from
Jan 4, 2023

Conversation

Tensho
Copy link
Contributor

@Tensho Tensho commented Dec 12, 2022

Description

The kubernetes_ingress_v1 resource has timeouts support, but it's not specified in resource schema definition and not reflected in the documentation.

Release Note

`resource/kubernetes_ingress_v1`: add create and delete timeouts

Test Outpus

Run basic test against kind cluster:

$ make testacc TESTARGS='-run=TestAccKubernetesIngressV1_serviceBackend'
==> Checking that code complies with gofmt requirements...
go vet .
TF_ACC=1 go test "/Users/tensho/Projects/oss/terraform-provider-kubernetes/kubernetes" -v -run=TestAccKubernetesIngressV1_serviceBackend -timeout 3h
=== RUN   TestAccKubernetesIngressV1_serviceBackend
--- PASS: TestAccKubernetesIngressV1_serviceBackend (4.82s)
PASS
ok  	github.com/hashicorp/terraform-provider-kubernetes/kubernetes	5.211s

Run wait for load balancer test against GKE cluster:

$ make testacc TESTARGS='-run=TestAccKubernetesIngressV1_WaitForLoadBalancer'
==> Checking that code complies with gofmt requirements...
go vet .
TF_ACC=1 go test "/Users/tensho/Projects/oss/terraform-provider-kubernetes/kubernetes" -v -run=TestAccKubernetesIngressV1_WaitForLoadBalancer -timeout 3h
=== RUN   TestAccKubernetesIngressV1_WaitForLoadBalancerGoogleCloud
--- PASS: TestAccKubernetesIngressV1_WaitForLoadBalancerGoogleCloud (1089.28s)
PASS
ok  	github.com/hashicorp/terraform-provider-kubernetes/kubernetes	1089.782s

References

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment

@Tensho Tensho force-pushed the doc-ingress-timeout branch from 0059557 to e6c50a2 Compare December 15, 2022 22:03
@Tensho Tensho changed the title Document kubernetes_ingress_v1 timeouts Add kubernetes_ingress_v1 timeouts Dec 15, 2022
@Tensho
Copy link
Contributor Author

Tensho commented Jan 3, 2023

@arybolovlev Please could you elaborate on why there is a binding to port 80 inside the ingress test (wait for load balancer)? This port requires root privileges while the container runs without it. I had to change it to something greater than 1024 to make the test pass.

Screenshot 2023-01-03 at 17 25 44

@arybolovlev
Copy link
Contributor

@arybolovlev Please could you elaborate on why there is a binding to port 80 inside the ingress test (wait for load balancer)? This port requires root privileges while the container runs without it. I had to change it to something greater than 1024 to make the test pass.

Screenshot 2023-01-03 at 17 25 44

Great catch!

This is a very good question and unfortunately I don't know the reason for that. What would you say if we use 8080 even though we know this is a default value for this image, but make it explicit?

It would be great if you can add this as well in addition to the great job you have already done. 👍🏻

Copy link
Contributor

@arybolovlev arybolovlev left a comment

Choose a reason for hiding this comment

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

All looks good, just one more query from my end.

Could you please add a changelog file so we can then auto-generate change log for the upcoming release?

It will be a simple text file in .changelog folder. Like this: .changelog/1936.txt. With the following content:

```release-note:enhancement
`resource/kubernetes_ingress_v1`: add create and delete timeouts
```

Thank you! 👍🏻

@github-actions github-actions bot added size/S and removed size/XS labels Jan 4, 2023
@Tensho
Copy link
Contributor Author

Tensho commented Jan 4, 2023

@arybolovlev Addressed all your concerns. Please let me know if there's anything else you can think of.

@arybolovlev arybolovlev self-requested a review January 4, 2023 09:37
Copy link
Contributor

@arybolovlev arybolovlev left a comment

Choose a reason for hiding this comment

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

Thank you, @Tensho!
🚀
Looks great! I will go ahead and merge your changes. They will be available in the upcoming release. Please keep an eye on the release notes!

Happy New Year! 🎉

@arybolovlev arybolovlev merged commit 08b70ef into hashicorp:main Jan 4, 2023
@Tensho Tensho deleted the doc-ingress-timeout branch January 4, 2023 09:53
@Tensho
Copy link
Contributor Author

Tensho commented Jan 4, 2023

Thank you for your guidance and quick response, @arybolovlev 🙇 Keeping an eye on releases.

@github-actions
Copy link

github-actions bot commented Feb 4, 2023

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants