-
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_app_job
Rename plural properties to singular for consistency with other Container App resources
#26063
Conversation
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.
Thanks for this PR @oWretch. I can see the value in this being consistent across the Container App resources, however given our backlog for 4.0 and the time frame in which we're planning to release it, we can't guarantee that someone in the team will get around to renaming this for 4.0.
I'm happy to merge this as is, just wanting to set expectations here. Alternatively you're welcome to give this a go yourself, we have some guidance in our contributor docs on how to deprecate and rename properties here.
… properties in 4.0
`secrets` to `secret` `registries` to `registry`
e1fe72d
to
718da53
Compare
Thanks @stephybun. I was being slightly cheeky and thinking a simple rename for 4.0 would solve the problem instead of extra code 🙂 I've updated the PR to deprecate the plural properties, and move over to the singular properties. |
azurerm_container_app_job
Add TODO to rename secrets
and registries
properties in 4.0azurerm_container_app_job
Rename plural properties to singular for consistency with other Container App resources
Note: I haven't included the logic to prevent deleting secrets that currently exists as it is now supported to remove secrets. The removal of this restriction exists in #25969. |
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.
Thanks so much for doing the deprecation here @oWretch!
In addition to the comments left in-line. We should also add a test for the deprecated properties to ensure that these still work with the changes. This test can be skipped using t.Skip
when features.FourPointOhBeta
is true.
Hi, subscribing to this thread (as the author of #25969). If this lands before my PR I will update the tests and other needed files. |
Tweak logic so it all works too
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.
Tests look good. Thanks for this @oWretch! LGTM 🦞
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. |
Community Note
Description
This PR just adds two TODOs to the
azurerm_container_app_job
resource to rename thesecrets
andregistries
properties from plural to singular to match the properties in theazurerm_container_app
resource for consistency.PR Checklist
Change Log
Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.
azurerm_container_app_job
- renamesecrets
tosecret
[azurerm_container_app_job
Rename plural properties to singular for consistency with other Container App resources #26063]azurerm_container_app_job
- renameregistries
toregistry
[azurerm_container_app_job
Rename plural properties to singular for consistency with other Container App resources #26063]Changes to existing Resource / Data Source
This is a (please select all that apply):