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

[Discussion] - Contributors guide new resource vs inline #19129

Merged
merged 5 commits into from
Dec 12, 2022

Conversation

aristosvo
Copy link
Collaborator

@aristosvo aristosvo commented Nov 3, 2022

Goal

To put my thoughts in a doc and start a little discussion, align practices, avoid unnecessary (re)work.

Why

I've been struggling a bit with this the last few days and also before when contributing resources/properties like:

Other resources like VNet/Subnets/NSGs have their troubles as well, not only from a contributor perspective but also from a user perspective with security boundaries between Infra Teams, Networking Teams, Application Teams etc. Not gonna solve that with this one and only doc, but maybe as a 'best practices' spin-off.

I'm not aware if there is any previous work done or talks on this topic, maybe a nice one for HashiTalks: Build.

Status

First Draft

@katbyte
Copy link
Collaborator

katbyte commented Nov 3, 2022

thats for opening this @aristosvo - and provided such a great writeup for the contrib docs

A going to drop in my comments from the other PR and a few thoughts:

IMHO i think a 1:1 relation alone is reason enough to consider adding functionality to the existing resource. Even more so when it really is just "enabling/configuring feature X on resource Y".

i think its a better user experience to have resource.feature_x_enabled then

resource "azurerm_something_feature_enabled" {
    something_id = numbers
    enabled = true
}

there are times where it does not make sense/can't be: ie CMK where the resource has to be created, permissions updated, and then CMK applied

or users may want to configure it separately (adding network ules)

or users may want to configure it in both places: azuread_group.members vs azuread_group_member

seems like you've covered most of this already with what you've written 😄

Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

Thanks again for this @aristosvo! - leaving some minor comments inline for when @tombuildsstuff loops back for a final look

contributing/topics/guide-new-resource-vs-inline.md Outdated Show resolved Hide resolved
contributing/topics/guide-new-resource-vs-inline.md Outdated Show resolved Hide resolved
contributing/topics/guide-new-resource-vs-inline.md Outdated Show resolved Hide resolved
@katbyte katbyte marked this pull request as ready for review December 12, 2022 05:58
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

LGTM 🍄

@katbyte katbyte merged commit 51823ee into hashicorp:main Dec 12, 2022
favoretti pushed a commit to favoretti/terraform-provider-azurerm that referenced this pull request Jan 12, 2023
@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 Jan 13, 2023
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.

3 participants