-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
azurerm_container_group: support exposed_port to configure ports at group level #10491
azurerm_container_group: support exposed_port to configure ports at group level #10491
Conversation
Hello, and thanks in advance for your understanding. Is there any chance I can get a review on this? I'm happy to make any necessary changes. I just don't want this one to fall through the cracks. Thanks again! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @amasover - Thanks for picking this back up again, it's greatly appreciated! It's off to a good start, I've left some comments and suggestions below to help move it forward. I'll circle back as soon as I can after they're addressed.
Thanks again!
azurerm/internal/services/containers/container_group_resource.go
Outdated
Show resolved
Hide resolved
Co-authored-by: Steve <[email protected]>
Co-authored-by: Steve <[email protected]>
…raform-provider-azurerm into container-group-ports-redux
…ndividual container
Hi @jackofallops thank you very much for the review. I think I've resolved most of your comments. The only outstanding issue is around schema.SchemaConfigModeAttr. I think I'm leaning toward removing it as you suggested, and putting guidance in the documentation that users taint existing resources if they wish to completely remove exposed_ports and return to the old behavior. Either way, I think this PR is ready for another look. Thanks again! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@amasover Thanks again for your work and patience on this PR, this is looking great.
Use of SchemaConfigModeAttr is a good choice here, it's the only way of enabling users to set a zero value for an Optional + Computed block. We wouldn't direct users to taint a resource, even if the property is ForceNew.
Tests are passing, and I believe this is now good to merge!
(one unrelated failure)
This has been released in version 2.57.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example: provider "azurerm" {
version = "~> 2.57.0"
}
# ... other configuration ... |
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 contributions. |
Fixes #4413
Previous PR & discussion: #5091
exposed_port
to allow configuring ports at group level without breaking backwards compatibility (field is marked as optional for now).exposed_port
is marked as both optional and computed, I've also setConfigMode: schema.SchemaConfigModeAttr
. Without this set, I don't think there would be a way to remove the lastexposed_port
block in a config and revert back to the old behavior (where the old behavior is thatports
on the container level are automatically exposed on the group level).Feedback is very much welcome.