fix: support reading GH files above 1MB #3007
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 |
iblankJC
force-pushed
the
support-larger-files
branch
from
3631fd8 to
b9e52ac
Compare
|
Hey @iblankJC 👋 Thank you for your contribution! It seems that this use-case has already been covered in the SDK. Could you change your approach to use the SDK |
deiga
left a comment
deiga
left a comment
There was a problem hiding this comment.
It seems that this use-case has already been covered in the SDK. Could you change your approach to use the SDK DownloadContents method instead? https://github.com/google/go-github/blob/v67.0.0/github/repos_contents.go#L139
iblankJC
force-pushed
the
support-larger-files
branch
from
8f6a30b to
e1a50a0
Compare
|
|
||
| var content string | ||
|
|
||
| if encoding == "" || encoding == "none" { |
There was a problem hiding this comment.
question: Can you link to documentation where it states that this is the best way to determine if DownloadContents should be used?
I would've thought that using fc.Size could have been a better indicator
| testCase := func(t *testing.T, mode string) { | ||
| resource.Test(t, resource.TestCase{ | ||
| PreCheck: func() { skipUnlessMode(t, mode) }, | ||
| Providers: testAccProviders, | ||
| Steps: []resource.TestStep{ | ||
| { | ||
| Config: initialConfig, | ||
| Check: initialCheck, | ||
| }, | ||
| { | ||
| Config: updatedConfig, | ||
| Check: updatedCheck, | ||
| }, | ||
| }, | ||
| }) | ||
| } | ||
|
|
||
| t.Run("with an anonymous account", func(t *testing.T) { | ||
| t.Skip("anonymous account not supported for this operation") | ||
| }) | ||
|
|
||
| t.Run("with an individual account", func(t *testing.T) { | ||
| testCase(t, individual) | ||
| }) | ||
|
|
||
| t.Run("with an organization account", func(t *testing.T) { | ||
| testCase(t, organization) | ||
| }) |
There was a problem hiding this comment.
We no longer use the testCase structure
| testCase := func(t *testing.T, mode string) { | |
| resource.Test(t, resource.TestCase{ | |
| PreCheck: func() { skipUnlessMode(t, mode) }, | |
| Providers: testAccProviders, | |
| Steps: []resource.TestStep{ | |
| { | |
| Config: initialConfig, | |
| Check: initialCheck, | |
| }, | |
| { | |
| Config: updatedConfig, | |
| Check: updatedCheck, | |
| }, | |
| }, | |
| }) | |
| } | |
| t.Run("with an anonymous account", func(t *testing.T) { | |
| t.Skip("anonymous account not supported for this operation") | |
| }) | |
| t.Run("with an individual account", func(t *testing.T) { | |
| testCase(t, individual) | |
| }) | |
| t.Run("with an organization account", func(t *testing.T) { | |
| testCase(t, organization) | |
| }) | |
| resource.Test(t, resource.TestCase{ | |
| PreCheck: func() { skipUnauthenticated(t) }, | |
| Providers: testAccProviders, | |
| Steps: []resource.TestStep{ | |
| { | |
| Config: initialConfig, | |
| Check: initialCheck, | |
| }, | |
| { | |
| Config: updatedConfig, | |
| Check: updatedCheck, | |
| }, | |
| }, | |
| }) |
| } | ||
| `, randomID, initialContent) | ||
|
|
||
| updatedConfig := fmt.Sprintf(` |
There was a problem hiding this comment.
suggestion: If this is equal to the string in initialConfig save for the updatedContent then it could make sense to reduce duplication here and extract the template string and reuse it
| initialCheck := resource.ComposeTestCheckFunc( | ||
| resource.TestCheckResourceAttr( | ||
| "github_repository_file.test", "content", | ||
| initialContent, | ||
| ), | ||
| resource.TestCheckResourceAttrSet( | ||
| "github_repository_file.test", "sha", | ||
| ), | ||
| resource.TestCheckResourceAttrSet( | ||
| "github_repository_file.test", "commit_sha", | ||
| ), | ||
| resource.TestCheckResourceAttr( | ||
| "github_repository_file.test", "file", | ||
| "large-file.txt", | ||
| ), | ||
| ) | ||
|
|
||
| updatedCheck := resource.ComposeTestCheckFunc( |
There was a problem hiding this comment.
issue: Please inline these, using variables is unnecessary obfuscation
2 participants
Resolves #2836
Before the change?
error: unsupported content encoding: noneAfter the change?
AcceptheaderPull request checklist
Does this introduce a breaking change?
Please see our docs on breaking changes to help!