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 option to set ManagedBy field. #22012

Merged
merged 1 commit into from
Jun 28, 2023
Merged

Conversation

rna-afk
Copy link
Contributor

@rna-afk rna-afk commented Jun 1, 2023

Adding the option to set the ManagedBy field to
the azure_resource_group resource.

@github-actions github-actions bot added the size/S label Jun 1, 2023
@tombuildsstuff
Copy link
Contributor

hey @rna-afk

Thanks for this PR.

Last time we looked into this, unfortunately the managedBy field couldn't be set by anything other than the Azure control-plane itself - as such this probably doesn't make sense to expose as a user-configurable field in Terraform.

However in the proposed changes this is being exposed as a read-only field (since it's only being set from the API Response), so I'm curious as to the use-case for this field - are you looking to use the value from this field in another resource, or are you looking to manage this field?

Thanks!

@rna-afk
Copy link
Contributor Author

rna-afk commented Jun 5, 2023

We are just looking to use this field for some automated system that we have running in the background that just checks the field and installs a few RBAC resources into the resource group if the right value is set. The almost immutable nature of managedby could be useful here.

Copy link
Member

@stephybun stephybun left a 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 @rna-afk. Looking into this it appears this field can be set.

In addition to the comments left below we need to add an additional acceptance test for this property and also to update the documentation. Once those points have been resolved we can get this merged. Thanks!

internal/services/resource/resource_group_data_source.go Outdated Show resolved Hide resolved
internal/managedby/schema.go Outdated Show resolved Hide resolved
internal/services/resource/resource_group_data_source.go Outdated Show resolved Hide resolved
internal/services/resource/resource_group_resource.go Outdated Show resolved Hide resolved
internal/services/resource/resource_group_resource.go Outdated Show resolved Hide resolved
internal/services/resource/resource_group_resource.go Outdated Show resolved Hide resolved
internal/services/resource/resource_group_resource.go Outdated Show resolved Hide resolved
@rna-afk
Copy link
Contributor Author

rna-afk commented Jun 20, 2023

Thanks for the suggestions @stephybun . I fixed them and added unit tests.

Copy link
Member

@stephybun stephybun left a comment

Choose a reason for hiding this comment

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

Thanks @rna-afk, one more comment and the docs still need to be updated.

internal/services/resource/resource_group_resource.go Outdated Show resolved Hide resolved
@rna-afk
Copy link
Contributor Author

rna-afk commented Jun 23, 2023

Thanks @stephybun, added docs.

@jackofallops jackofallops self-assigned this Jun 26, 2023
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.

Thanks for this contribution @rna-afk

I've left a few comments below if you can take a look, then we can get this tested.

Thanks!

internal/services/resource/resource_group_data_source.go Outdated Show resolved Hide resolved
internal/services/resource/resource_group_resource.go Outdated Show resolved Hide resolved
website/docs/r/resource_group.html.markdown Outdated Show resolved Hide resolved
website/docs/r/resource_group.html.markdown Outdated Show resolved Hide resolved
Adding the option to set the ManagedBy field to
the `azure_resource_group` resource.
Copy link
Member

@stephybun stephybun left a comment

Choose a reason for hiding this comment

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

Thanks for this @rna-afk LGTM 🌻

@stephybun stephybun merged commit 532b628 into hashicorp:main Jun 28, 2023
@github-actions github-actions bot added this to the v3.63.0 milestone Jun 28, 2023
stephybun added a commit that referenced this pull request Jun 28, 2023
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 24, 2024
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.

4 participants