Skip to content

fix: change fork to TypeString and add conditional ForceNew#2959

Merged
diofeher merged 5 commits intointegrations:mainfrom
diofeher:bug/repository_fork
Dec 3, 2025
Merged

fix: change fork to TypeString and add conditional ForceNew#2959
diofeher merged 5 commits intointegrations:mainfrom
diofeher:bug/repository_fork

Conversation

@diofeher
Copy link
Copy Markdown
Collaborator

@diofeher diofeher commented Dec 2, 2025

Resolves #2954


Before the change?

  • Previously, resources tracked on the state before feat(core): add fork functionality #2678, didn't have the fork field. Now that we're supporting that, any resource that adds the attribute triggers recreation, even if the attribute is the same as the one returned by the API.

After the change?

  • We're adding Computed to the field, so the resource has access to the value to calculate if it needs to recreate the resource. By using that in fork, the other two attributes are filled in.

Pull request checklist

  • Schema migrations have been created if needed (example)
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)

Does this introduce a breaking change?

Please see our docs on breaking changes to help!

  • Yes
  • No

@github-actions github-actions bot added the Type: Bug Something isn't working as documented label Dec 2, 2025
@diofeher diofeher self-assigned this Dec 2, 2025
@diofeher diofeher changed the title fix: forcenew only if the value is not nil fix: forcenew only if the value is not nil and different from previous value Dec 2, 2025
@diofeher diofeher changed the title fix: forcenew only if the value is not nil and different from previous value fix: add computed to a few attributes in resource_github_repository Dec 3, 2025
@diofeher diofeher force-pushed the bug/repository_fork branch from c5da0c5 to 62333fb Compare December 3, 2025 00:08
@diofeher
Copy link
Copy Markdown
Collaborator Author

diofeher commented Dec 3, 2025

I wasn't able to find a way to automate the test as I was doing it manually. It would be helpful to have another round of testing, though.

@diofeher diofeher marked this pull request as ready for review December 3, 2025 02:34
@diofeher diofeher requested a review from nickfloyd December 3, 2025 02:35
@diofeher diofeher changed the title fix: add computed to a few attributes in resource_github_repository fix: add computed to fork in resource_github_repository Dec 3, 2025
@diofeher diofeher changed the title fix: add computed to fork in resource_github_repository fix: add Computed to fork in resource_github_repository Dec 3, 2025
@diofeher diofeher force-pushed the bug/repository_fork branch from 2067b0c to dc71e81 Compare December 3, 2025 04:42
@diofeher diofeher force-pushed the bug/repository_fork branch from dc71e81 to c8478e8 Compare December 3, 2025 04:44
@diofeher diofeher changed the title fix: add Computed to fork in resource_github_repository fix: change fork to TypeString Dec 3, 2025
@deiga
Copy link
Copy Markdown
Collaborator

deiga commented Dec 3, 2025

I think we might need to do the same to the source_owner and source_repo fields too

Signed-off-by: Diogenes Fernandes <diofeher@gmail.com>
@diofeher
Copy link
Copy Markdown
Collaborator Author

diofeher commented Dec 3, 2025

@deiga Thanks for the catch! I previously added to them but removed them in later iterations.

So just using the Computed field is enough, since the read call is populating these two fields, at least that's what I've seen with my latest tests.

@diofeher
Copy link
Copy Markdown
Collaborator Author

diofeher commented Dec 3, 2025

I tested migrating from the existing field "TypeBool" to "TypeString," and it also worked well.

Copy link
Copy Markdown
Member

@nickfloyd nickfloyd left a comment

Choose a reason for hiding this comment

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

It looks like we still need ForceNewIfChange for both source_owner and source_repo.

Signed-off-by: Diogenes Fernandes <diofeher@gmail.com>
Signed-off-by: Diogenes Fernandes <diofeher@gmail.com>
@diofeher diofeher changed the title fix: change fork to TypeString fix: change fork to TypeString and add conditional ForceNew Dec 3, 2025
@diofeher
Copy link
Copy Markdown
Collaborator Author

diofeher commented Dec 3, 2025

Thanks for the reviews @deiga and @nickfloyd

Signed-off-by: Diogenes Fernandes <diofeher@gmail.com>
return oldValStr != "" && oldValStr != newValStr
}

func customDiffFunction(_ context.Context, diff *schema.ResourceDiff, v any) error {
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I'm moving this function closer to where it's used so it's easier to read and navigate.

@diofeher diofeher merged commit 9b08c04 into integrations:main Dec 3, 2025
8 checks passed
AlanCitrix pushed a commit to AlanCitrix/terraform-provider-github that referenced this pull request Dec 10, 2025
…grations#2959)

* fix: change "fork" to TypeString

* lint + computed on source_owner and source_repo

Signed-off-by: Diogenes Fernandes <diofeher@gmail.com>

* adding automated test

Signed-off-by: Diogenes Fernandes <diofeher@gmail.com>

* source_repo and source_owner logic added

Signed-off-by: Diogenes Fernandes <diofeher@gmail.com>

* lint

Signed-off-by: Diogenes Fernandes <diofeher@gmail.com>

---------

Signed-off-by: Diogenes Fernandes <diofeher@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Type: Bug Something isn't working as documented

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG]: Pre-existing fork repositories are recreated when fork set to true

3 participants