feat: add github_repository_custom_properties resource for batch property management#3237
Conversation
|
👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labeled with |
|
@deiga / @stevehipwell , any chances for reviewing this PR? |
|
@mkushakov please don't ping us multiple times. If you currently have capacity for these PRs maybe you could review the last few merged PRs to look at what are things we usually ask for? |
sorry, don't want to be PITA, just want to see what is a way forward to backport most important changes from our fork so we can move back to this upstream |
New resource for managing multiple custom property values on a repository in a single resource block with in-place updates. Complements the existing singular github_repository_custom_property resource.
dee9827 to
fd4c6dd
Compare
- Use context-aware CRUD functions (CreateContext, ReadContext, etc.) - Rename repository_name to repository, add computed repository_id field - Add CustomizeDiff with diffRepository for repo rename support - Separate Create and Update into distinct functions - Use buildID/parseID2 with : separator instead of buildTwoPartID - Replace log.Printf with tflog for structured logging - Return d.Set errors instead of swallowing them - Extract filterManagedCustomProperties to reduce cognitive complexity - Update tests to use ConfigStateChecks and extract duplicate literals - Update import format to accept repository name (owner from provider) - Update documentation to reflect schema changes
fd4c6dd to
e6829dc
Compare
…tom-properties-resource
| client := meta.(*Owner).v3client | ||
| owner := meta.(*Owner).name | ||
|
|
||
| _, repoName, err := parseID2(d.Id()) |
There was a problem hiding this comment.
suggestion: Instead of parsing the repo name from Id it's recommended to use d.Get the field since it exists
| return diag.FromErr(fmt.Errorf("error reading custom properties for repository %s/%s: %w", owner, repoName, err)) | ||
| } | ||
|
|
||
| managedProperties, err := filterManagedCustomProperties(allCustomProperties, managedPropertyNames, isImport) |
There was a problem hiding this comment.
issue: this makes the resource non-authoritative which can encourage patterns that cause constant drift and confusion.
What do you think about this?
There was a problem hiding this comment.
The resource is intentionally non-authoritative: it only manages the properties explicitly declared in the property {} blocks and ignores everything else on the repository.
Why non-authoritative:
- Organizations often have custom properties managed by multiple teams, tools, or projects. Authoritative behavior would force all properties into a single resource block, which is impractical and fragile.
- An authoritative resource that deletes unmanaged properties on apply can be destructive, especially during gradual adoption.
- This matches how the existing singular
github_repository_custom_propertyalready works — it doesn't touch other properties either. - Many mature providers (e.g. GCP IAM bindings, AWS) offer non-authoritative variants for exactly this reason.
Regarding drift: only the declared properties are tracked, so there's no "phantom drift" from externally managed properties. If someone changes a managed property outside Terraform, the next plan correctly detects and fixes it.
I've updated the documentation and the resource description to explicitly call out the non-authoritative behavior so it's clear to users. See this commit.
| // Read actual properties from GitHub | ||
| allCustomProperties, _, err := client.Repositories.GetAllCustomPropertyValues(ctx, owner, repoName) | ||
| if err != nil { | ||
| return diag.FromErr(fmt.Errorf("error reading custom properties for repository %s/%s: %w", owner, repoName, err)) |
| // If no properties exist, remove resource from state | ||
| if len(managedProperties) == 0 { | ||
| tflog.Warn(ctx, "No custom properties found, removing from state", map[string]any{"owner": owner, "repository": repoName}) | ||
| d.SetId("") | ||
| return nil | ||
| } |
There was a problem hiding this comment.
question: I'm wondering if this makes sense as it will cause the resource to be recreated unnecessarily?
If this was an authorative resource I could fathom the purpose.
Could you elaborate on the use-case here?
There was a problem hiding this comment.
This guard handles the edge case where all managed properties have been removed externally (e.g., the org-level property definitions were deleted). Since the resource has nothing left to manage, removing it from state makes Terraform surface the discrepancy on the next plan — the user sees the resource will be recreated, which prompts them to either fix the org definitions or remove the resource from their config.
Without this, the Read would silently succeed with an empty property set, which would conflict with the MinItems: 1 constraint and could cause confusing plan output.
That said, I'm open to alternative approaches — e.g., returning a warning diagnostic instead of removing from state, if you think that would be less surprising.
There was a problem hiding this comment.
issue: Using SetSizeExact in the tests is hazardous as the resource is non-authorative. It's possible that there are other properties configured in the Test Org
| }, | ||
| { | ||
| Config: configUpdate, | ||
| ConfigStateChecks: []statecheck.StateCheck{ |
There was a problem hiding this comment.
issue: Please add Plan checks to verify that it does in-place updates https://developer.hashicorp.com/terraform/plugin/testing/acceptance-tests/plan-checks/resource
| Steps: []resource.TestStep{ | ||
| { | ||
| Config: config, | ||
| ConfigStateChecks: []statecheck.StateCheck{ |
There was a problem hiding this comment.
issue: The checks don't verify that it's a multi-select property, should they?
| if err != nil { | ||
| return diag.FromErr(fmt.Errorf("error deleting custom properties for repository %s/%s: %w", owner, repoName, err)) | ||
| } |
There was a problem hiding this comment.
issue: This also needs to check if the repo is deleted or archived, in which case the error is not an error
| properties := d.Get("property").(*schema.Set).List() | ||
| if len(properties) == 0 { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
question: What use case does this guard against? The Schema defines property as min 1 item, so we should never have 0 properties, right?
…r and import management
…improve error handling Co-authored-by: Copilot <[email protected]>
…ory not found scenarios Co-authored-by: Copilot <[email protected]>
Resolves #3240
Summary
Add a new
github_repository_custom_propertiesresource (plural) that manages multiple custom property values on a single repository in one resource block, with in-place updates when values change.This complements the existing
github_repository_custom_propertyresource (singular), which manages one property per resource instance and requires destroy+recreate on any change.Motivation
The existing singular
github_repository_custom_propertyresource has limitations:PATCHproperty_type— the user must manually specify the property type, even though it's already defined at the org levelThe new plural resource addresses all of these:
github_repository_custom_property)github_repository_custom_properties)property_typeowner/repo/property_nameowner/repo(imports ALL properties)Changes
New Resource (
resource_github_repository_custom_properties.go)repository_name(ForceNew) andproperty(TypeSet of name/value blocks)CreateandUpdateshare the same function — looks up org property definitions to auto-detect types, then callsCreateOrUpdateCustomPropertieswith all properties in a single API callReadfilters to only managed properties (from state), or imports ALL properties when state is empty (import case)Deletesets all managed properties tonilin a single API callStateContextimporter that acceptsowner/repoformat and imports all propertiescheckOrganizationguard on all CRUD operationsfmt.Errorfand%wProvider Registration (
provider.go)github_repository_custom_propertiesTests (
resource_github_repository_custom_properties_test.go)owner/repoAll tests use
providerFactories(modern pattern) andskipUnlessHasOrgs.Example Usage
Relationship to Existing Resources
github_repository_custom_property(singular) — unchanged, still available. Users can choose whichever pattern fits their workflow.github_organization_custom_properties— defines properties at the org level. The new resource reads these definitions to auto-detect property types.data.github_repository_custom_properties(data source) — already exists, reads all properties for a repo.Testing
go build ./...— passesgo vet ./...— passes