chore: Deprecate duplicate org team role resources #2933
Conversation
|
|
||
| func resourceGithubOrganizationRoleTeamAssignment() *schema.Resource { | ||
| return &schema.Resource{ | ||
| DeprecationMessage: "This resource is deprecated in favor of the github_organization_role_team resource.", |
There was a problem hiding this comment.
question: Do we have a naming convention for these kinds of resources? I personally think that github_organization_role_team_assignment is clearer than github_organization_role_team, especially since GitHub uses the "Role assignment" terminology
And a second note: The docs for https://registry.terraform.io/providers/integrations/github/latest/docs/resources/organization_role_team_assignment are more verbose and user-friendly than https://registry.terraform.io/providers/integrations/github/latest/docs/resources/organization_role_team
So, if we want to deprecate github_organization_role_team_assignment can we copy over the docs from it? :)
There was a problem hiding this comment.
Naming for the resources and data is a good thing to have.
There is for example another thing which may need to be addressed as well due to naming.
The data and resource for organization custom properties are named in plural properties, but neither data not resource works with more than a single property.
The resource organization_custom_properties and data organization_custom_properties
It means it most likely needs to be done depreciation of these "plurals" and introducing of "singulars" with property in the names.
It could be that "singular" named resources should be introduced in v6 and "plurals" removed in v7.
I saw @mroyme done naming of repsource with singular property in their fork, which is more correct if it adds only a single custom property.
PS Added the issue #2936
There was a problem hiding this comment.
@deiga the resource github_organization_role_team was implemented with all of the other related resources and data sources using consistent naming and implementation patterns.
I also don't agree that the assignment suffix is necessarily an improvement to the name, it adds additional characters to parse and fit on a single line. In this context the point is consistency with all of the other related resources. If you want to discuss changing the naming conventions I think that's a separate issue.
@ViacheslavKudinov I agree with your points on pluralization and duplication of context in names. I'm not sure which way you're arguing here but in this context my interpretation of your logic would be that the assignment suffix isn't required as the context is already complete.
There was a problem hiding this comment.
i have no strong opinion about this concrete resource, tbh.
PS At the same time as it was introduced Enterprise Roles and (Custom) Enterprise Role can be assigned to Enterprise team(s), it would be great if we use same "naming" for those as well to keep consistency, when we introduce new data/resources.
There was a problem hiding this comment.
PS At the same time as it was introduced Enterprise Roles and (Custom) Enterprise Role can be assigned to Enterprise team(s), it would be great if we use same "naming" for those as well to keep consistency, when we introduce new data/resources.
FYI the PR (#2487) was opened almost a year ago, long before enterprise roles were a thing.
There was a problem hiding this comment.
@stevehipwell thanks for sharing.
Now it looks like (from my perspective) is better to deprecate organization_role_team_assignment, exactly what PR addresses (it was introduced a little bit more than a month ago in v6.7.0.
As it (organization_role_team_assignment) was added "recently" may need less changes for provider consumers instead of organization_role_team, which is present for almost one year.
There was a problem hiding this comment.
@ViacheslavKudinov they were all introduced in the same release, the PR freeze left everything backed up which meant that overlapping PRs got merged. The organization_role_team resource is part of a set of 11 resources or data sources while organization_role_team_assignment was in a PR on it's own.
There was a problem hiding this comment.
@stevehipwell thank you for sharing the context.
I totally fine with this PR as I got better understanding. Have no any concern.
Signed-off-by: Steve Hipwell <steve.hipwell@gmail.com>
stevehipwell
force-pushed
the
org-role-team-assignment-deprecation
branch
from
390f79b to
8f4dd75
Compare
|
@nickfloyd do we have a v6 release branch (e.g. |
Hey @stevehipwell help me understand the thinking behind the need to cherry pick our changes into a v6.x branch? Currently main is clean and shippable for any future v6.x changes - no breaking changes have been introduced in main and we should be able to treat the last v6.x release as just a normal release with one caveat - we've finally create a v6 release branch before merging anything v7 into main. I could definitely be missing something in the line of thinking though - I want to make sure have a complete understanding. |
|
@nickfloyd I was assuming that your v77 branch was heading to |
|
@stevehipwell Got it, thank you for the clarity. I was trying to hold off on that as long as we possibly could to avoid the additional complexity of maintaining two "main" branches. I know that it will cause extra work on me on the other side but I'd prefer to simply roll as much as we can from this milestone targeting a merge of v77 next week. |
4 participants
Resolves #ISSUE_NUMBER
Before the change?
There are multiple resources for managing organization role teams and the deprecated security managers API is still in use.
After the change?
I've deprecated the duplicate resources and the data source using the deprecated API.
Pull request checklist
Does this introduce a breaking change?
Please see our docs on breaking changes to help!