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

fix: Remove timeout for metal vrf #386

Merged
merged 3 commits into from
Sep 25, 2023

Conversation

aayushrangwala
Copy link
Contributor

@aayushrangwala aayushrangwala commented Sep 20, 2023

As part of fixing the timeouts for metal resources to actually comply with what is documented, it was found out that we donot document any timeout support for metal VRF whereas in the code we do have support for timeouts.

Since it has never been working due to the below issue, also not documented, it'll be ideal to remove it directly considering a code smell

Relates to: #357

@aayushrangwala aayushrangwala temporarily deployed to external September 20, 2023 14:50 — with GitHub Actions Inactive
@aayushrangwala aayushrangwala changed the title Timeout fix metal vrf fix: Timeout for metal vrf Sep 20, 2023
)

func resourceMetalVRF() *schema.Resource {
return &schema.Resource{
Timeouts: &schema.ResourceTimeout{
Copy link
Contributor

Choose a reason for hiding this comment

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

This is removing timeouts entirely rather than fixing them. I don't necessarily think that's an issue: as I understand it, they didn't work and they weren't documented, so IMO it's just as valid to remove them as it is to make them work. However, if we're removing them, then the PR title should be updated to reflect that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes thats exactly correct, added the description in PR description as well

@aayushrangwala aayushrangwala requested a deployment to external September 22, 2023 15:41 — with GitHub Actions Abandoned
@aayushrangwala aayushrangwala changed the title fix: Timeout for metal vrf fix: Remove timeout for metal vrf Sep 23, 2023
@ctreatma ctreatma merged commit 45e8380 into equinix:main Sep 25, 2023
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.

2 participants