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

Update data source for repository file: prevent tf panic on non-existent file and make the branch default consistent with GitHub API #1129

Merged
merged 6 commits into from
Mar 31, 2023

Conversation

galargh
Copy link
Contributor

@galargh galargh commented Apr 28, 2022

Resolves #1132
Resolves #1131

Description

This PR updates the data source for repository files.

It prevents terraform from panicking when trying to retrieve info about a non-existent file. With this change, in such a case, an empty data source will get returned - similarly to how ref data source behaves.

It also changes the default value for branch argument from main to the name of the default branch of the requested repository. The new behaviour is how GitHub API itself behaves.

I also added a new attribute ref which, after apply, holds the name of the branch the file is associated with. In practice, if a branch was explicitly provided, then ref = branch. Otherwise, ref = the name of the default branch of the requested repository.

Finally, it simplified the commit info retrieval. It was possible because I noticed that commit_sha is never set on the data source - I suspect it was copied over from the resource and just left in place.

I updated the markdown entry which describes how to configure the resource.

Testing
  • TF_ACC=1 go test -v ./... -run ^TestAccGithubRepositoryFileDataSource

@douernesto
Copy link

This would solve a lot of issues!

@kfcampbell
Copy link
Member

@galargh do you have any interest in resolving this merge conflicts? I can do a pass at it if you'd prefer as well.

@galargh
Copy link
Contributor Author

galargh commented Feb 21, 2023

@galargh do you have any interest in resolving this merge conflicts? I can do a pass at it if you'd prefer as well.

Sure, no problem. All conflicts should be resolved now.

@kfcampbell
Copy link
Member

Hmm, tests for TestDataSourceGithubRepositoryFileRead are failing for me. Here is some example test output:

    data_source_github_repository_file_test.go:373: 
        	Error Trace:	/home/kfcampbell/github/dev/terraform-provider-github/github/data_source_github_repository_file_test.go:373
        	Error:      	Not equal: 
        	            	expected: "here-goes-content-of-our-glorious-config.json"
        	            	actual  : ""
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1 +1 @@
        	            	-here-goes-content-of-our-glorious-config.json
        	            	+
        	Test:       	TestDataSourceGithubRepositoryFileRead/using_user_as_owner_if_just_name_is_passed
    data_source_github_repository_file_test.go:374: 
        	Error Trace:	/home/kfcampbell/github/dev/terraform-provider-github/github/data_source_github_repository_file_test.go:374
        	Error:      	Not equal: 
        	            	expected: "test-repo/test-file.json"
        	            	actual  : ""
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1 +1 @@
        	            	-test-repo/test-file.json
        	            	+
        	Test:       	TestDataSourceGithubRepositoryFileRead/using_user_as_owner_if_just_name_is_passed

Can you reproduce this?

@galargh
Copy link
Contributor Author

galargh commented Feb 28, 2023

I've just realised I was only running TestAccGithubRepositoryFileDataSource before 🤦

I now fixed [TestDataSourceGithubRepositoryFileRead](https://github.com/integrations/terraform-provider-github/pull/1129/commits/1a8161ff9d5e3529e5b1c6cafac8a370adb59d98) too. I needed to update GitHub client mock responses which totally makes sense since the ds is making slightly different requests now.

I also ran go fmt on modified files.

Copy link
Member

@kfcampbell kfcampbell left a comment

Choose a reason for hiding this comment

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

@galargh thank you so much for contributing and following up! I'll get this merged and released shortly.

@kfcampbell kfcampbell merged commit fc0f5ba into integrations:main Mar 31, 2023
avidspartan1 pushed a commit to avidspartan1/terraform-provider-github that referenced this pull request Feb 5, 2024
…ent file and make the branch default consistent with GitHub API (integrations#1129)

* Update data source for repository file.

* Create randomID variable for use in tests

* fix: TestDataSourceGithubRepositoryFileRead

---------

Co-authored-by: Keegan Campbell <me@kfcampbell.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants