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

azurerm_container_group: support exposed_port to configure ports at group level #10491

Merged
merged 7 commits into from
Apr 30, 2021

Conversation

amasover
Copy link
Contributor

@amasover amasover commented Feb 5, 2021

Fixes #4413

Previous PR & discussion: #5091

  • Support exposed_port to allow configuring ports at group level without breaking backwards compatibility (field is marked as optional for now).
  • Path to mark as required in 3.0 of the provider.
  • Note: Because exposed_port is marked as both optional and computed, I've also set ConfigMode: schema.SchemaConfigModeAttr. Without this set, I don't think there would be a way to remove the last exposed_port block in a config and revert back to the old behavior (where the old behavior is that ports on the container level are automatically exposed on the group level).

Feedback is very much welcome.

@amasover
Copy link
Contributor Author

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!

@jackofallops jackofallops added this to the v2.57.0 milestone Apr 15, 2021
Copy link
Member

@jackofallops jackofallops left a 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!

@ghost ghost added size/L and removed size/M labels Apr 20, 2021
@amasover
Copy link
Contributor Author

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!

@ghost ghost removed the waiting-response label Apr 21, 2021
@manicminer manicminer self-requested a review April 29, 2021 23:04
Copy link
Contributor

@manicminer manicminer left a 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!

Screenshot 2021-04-30 at 00 56 50

(one unrelated failure)

@manicminer manicminer merged commit e06c27d into hashicorp:master Apr 30, 2021
manicminer added a commit that referenced this pull request Apr 30, 2021
@ghost
Copy link

ghost commented Apr 30, 2021

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 ...

@github-actions
Copy link

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.
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 May 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants