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

Add Overwrite Support For github_repository_file #459

Merged
merged 21 commits into from
Oct 3, 2020

Conversation

jcudit
Copy link
Contributor

@jcudit jcudit commented May 14, 2020

@ghost ghost added size/M Type: Documentation Improvements or additions to documentation labels May 14, 2020
Comment on lines 336 to 340
hardCodedRepoName := "test-repo"
return testAccCheckGithubRepositoryFileExistsWithRepo(n, path, branch, hardCodedRepoName, content, commit)
}

func testAccCheckGithubRepositoryFileExistsWithRepo(n, path, branch, repo string, content *github.RepositoryContent, commit *github.RepositoryCommit) resource.TestCheckFunc {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went down the path of removing references to "test-repo" and leveraging a per-test generated repository instead. Those changes turned out to break tests so I've opted to leave the existing code paths alone and expose testAccCheckGithubRepositoryFileExistsWithRepo instead. This allows for creating a repository from a template and overwriting its files during the test suite.

@jcudit jcudit requested a review from anGie44 May 14, 2020 21:54
@jcudit jcudit added this to the v2.8.0 milestone May 14, 2020
@jcudit jcudit changed the title Add overwrite support to github_repository_file Add Overwrite Support For github_repository_file May 14, 2020
@jcudit jcudit marked this pull request as ready for review May 14, 2020 22:19
Copy link
Contributor

@anGie44 anGie44 left a comment

Choose a reason for hiding this comment

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

do we expect the behavior of the overwrite to extend to updates as well? i'm thinking in the event we create a new file (assuming it doesn't already exist in a repo), if we go to update the config and overwrite is omitted or even explicitly set to false, the file will still get updated in place


func testAccGithubRepositoryFileOverwriteDisabledConfig(randString string) string {

owner := os.Getenv("GITHUB_ORGANIZATION")
Copy link
Contributor

Choose a reason for hiding this comment

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

the global testOrganization could also be used here

@anGie44 anGie44 modified the milestones: v2.8.0, v2.8.1 May 15, 2020
@jcudit
Copy link
Contributor Author

jcudit commented May 16, 2020

Thanks for raising discussion on the update case. I had not given it too much thought.

After thinking about this a bit, I recommend we rebrand overwrite to overwrite_on_create.
We can then assume that whenever the update code path is executed, we will always be overwriting a file, which matches the existing implementation.

To further safeguard, we can add the ForceNew attribute to overwrite_on_create so that if configuration changes, we can start fresh with a new file. What do you think @anGie44 ?

@jcudit
Copy link
Contributor Author

jcudit commented May 16, 2020

q

@jcudit jcudit closed this May 16, 2020
@jcudit jcudit reopened this May 16, 2020
@jcudit jcudit modified the milestones: v2.9.0, v2.10.0 Jun 3, 2020
@jaxxstorm
Copy link

Is there any news on when this might be reviewed/merged?

@anGie44
Copy link
Contributor

anGie44 commented Jun 9, 2020

I recommend we rebrand overwrite to overwrite_on_create.

I like this idea, suits the intended behavior 👍 my only hesitation would be adding the ForceNew attribute since the field is intended to be used only at create time and it might be more of a pain point to force recreation for users

@jcudit jcudit modified the milestones: v2.10.0, v3.1.0 Jul 10, 2020
@jcudit jcudit force-pushed the github_repository_file_overwrite branch from 546c27c to a0a3dcf Compare October 3, 2020 12:30
@ghost ghost added size/XL and removed size/M labels Oct 3, 2020
@jcudit jcudit force-pushed the github_repository_file_overwrite branch from a0a3dcf to c9f6353 Compare October 3, 2020 12:31
@ghost ghost added size/L and removed size/XL labels Oct 3, 2020
@ghost ghost added size/XL and removed size/L labels Oct 3, 2020
@ghost ghost added size/L and removed size/XL labels Oct 3, 2020
@jcudit
Copy link
Contributor Author

jcudit commented Oct 3, 2020

Tests are looking good. The GHES ones are failing as the infrastructure is down at the moment.

@jcudit jcudit merged commit 7fabb77 into master Oct 3, 2020
@jcudit jcudit deleted the github_repository_file_overwrite branch October 3, 2020 21:18
kfcampbell pushed a commit to kfcampbell/terraform-provider-github that referenced this pull request Jul 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/L Type: Documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

github_repository_file cant update/overwrite existing files.
3 participants