fix: Refactor repository custom property#3476
Conversation
stevehipwell
added
the
Type: Maintenance
|
👋 Hi, and thank you for this contribution! This repo is maintained by GitHub and community members on a best-effort basis. We'll get to this as soon as we can. You can help us prioritize by joining the discussion on open issues and PRs, sharing details on the changes you need, and reviewing other contributions. 🤖 This is an automated message. |
stevehipwell
force-pushed
the
refactor-repo-custom-prop
branch
from
db61ca4 to
c327081
Compare
There was a problem hiding this comment.
Pull request overview
These provider review instructions are being used. This PR refactors github_repository_custom_property to match newer provider patterns (context-aware CRUD, schema/state migration, tfplugindocs-generated docs/examples) while addressing prior bugs around import and in-place value updates.
Changes:
- Refactors
github_repository_custom_propertyto SDKv2*ContextCRUD, addsUpdateContext,diffRepository, and a v0→v1 state upgrader forrepository_id. - Updates generated docs/templates and adds example + import snippets for the resource.
- Expands acceptance coverage for additional value types/import and adds input validation for empty strings.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| templates/resources/repository_custom_property.md.tmpl | Switches resource doc template to tfplugindocs-driven schema/examples/import sections. |
| github/resource_github_repository_custom_property.go | Refactors resource to new CRUD/import/migration patterns; adds update support and validation. |
| github/resource_github_repository_custom_property_test.go | Updates/extends acceptance tests (new URL test, import test, validation test). |
| github/resource_github_repository_custom_property_migration.go | Adds v0 schema + state upgrader to populate repository_id. |
| github/resource_github_repository_custom_property_migration_test.go | Adds a placeholder (currently commented) migration test scaffold. |
| examples/resources/github_repository_custom_property/resource_1.tf | Updates example snippet used by docs. |
| examples/resources/github_repository_custom_property/import.sh | Adds terraform import example for docs. |
| examples/resources/github_repository_custom_property/import-by-string-id.tf | Adds import { id = ... } example for docs. |
| docs/resources/repository_custom_property.md | Updates generated resource docs to new template/schema/import format. |
| ARCHITECTURE.md | Updates architecture guidance snippet to reflect the newer resource structure patterns. |
Comments suppressed due to low confidence (1)
examples/resources/github_repository_custom_property/resource_1.tf:8
- The example names the resource
github_repository_custom_property.string, but the generated import examples usegithub_repository_custom_property.example. Keeping these consistent avoids confusion when users copy/paste the example + import snippet from the docs.
stevehipwell
force-pushed
the
refactor-repo-custom-prop
branch
from
d21951d to
c34d74e
Compare
|
@deiga I've fixed your review comments. |
stevehipwell
force-pushed
the
refactor-repo-custom-prop
branch
from
37e4a02 to
306b839
Compare
robert-crandall
left a comment
robert-crandall
left a comment
There was a problem hiding this comment.
Really nice refactor!
| repo, _, err := client.Repositories.Get(ctx, owner, repoName) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to retrieve repository %s: %w", repoName, err) | ||
| } |
There was a problem hiding this comment.
How would this handle a 404 if the repository is deleted? Would this require manually removing the repo from terraform state?
There was a problem hiding this comment.
Yeah, that's how it currently works in the provider. I don't really have the time to look into changing this now, but it might be that we could relax this if we can clear the ID in the migration; but this would require actual testing so a lot of effort.
stevehipwell
force-pushed
the
refactor-repo-custom-prop
branch
from
0a0c1d8 to
60f0086
Compare
|
@deiga this one is just waiting on you. |
| PlanOnly: true, | ||
| }, | ||
| { | ||
| Config: fmt.Sprintf(config, fmt.Sprintf(`["%s"]`, allowed[0])), |
There was a problem hiding this comment.
issue: this is getting hard to parse 😬
There was a problem hiding this comment.
I think it's easier to parse what's going on now that it was before, and there is no way to remove required complexity.
There was a problem hiding this comment.
"Fetch the first element of this array and build a string representation of an array with that element then insert that into a string representation of the TF config" IMO requires a few mental hops too many.
You don't need to change this, just highlighting that others than you might have it harder with this.
I would extract the nested fmt call into a constant just to make it easier to read.
But I know that gophers have different opinions about what is easy to read 😂
Signed-off-by: Steve Hipwell <steve.hipwell@gmail.com>
stevehipwell
force-pushed
the
refactor-repo-custom-prop
branch
from
e0b46ed to
f02503b
Compare
|
@deiga have you seen my responses? |
|
|
||
| tflog.Debug(ctx, "Migrating GitHub Repository Custom Property from v0 to v1.", map[string]any{"raw_state": rawState}) | ||
|
|
||
| state, err := migrateRepositoryWithID(ctx, client, owner, rawState) |
There was a problem hiding this comment.
I really like this pattern!
Resolves #3358
Resolves #3377
Resolves #2639
Closes #3387
Closes #3378
Closes #2640
Before the change?
github_repository_custom_propertyhad a number of bugs and was using the outdated patternAfter the change?
github_repository_custom_propertyhas been updated to the latest patternPull request checklist
Does this introduce a breaking change?
Please see our docs on breaking changes to help!