feat: add github_release_asset data source#2514
feat: add github_release_asset data source#2514stevehipwell merged 20 commits intointegrations:mainfrom
Conversation
dac6b1b to
696957f
Compare
|
Hi @lafrenierejm - This has been waiting on reviews for ~6 months. Is there anything I can do to help steward this along to get it reviewed, merged, and released? |
|
Hi @kfcampbell - I believe you've merged/released other PRs. Is there anything I can do to help steward this along to get it reviewed, merged, and released? Thanks! |
5ba605e to
0671ab4
Compare
|
Hey @mdb 👋 Thanks for the contribution! The changes look good for the most part.
|
9b364b5 to
2407ae3
Compare
Per request of @deiga (integrations#2514 (comment)), this adds tests for previewHeaderInjectorTransport.RoundTrip logic around application/octest-stream header handling. See google/go-github#3392 for context. Signed-off-by: Mike Ball <mikedball@gmail.com>
|
Thanks for the quick response @deiga !
I've added a comment linking to corresponding context over in google/go-github#3392 (and note that this is fixed in go-github/v68 (Admittedly, this is all a bit foggy for me -- it's been over a year since I opened this PR as well as since I opened google/go-github#3392)).
I added unit tests. Does that address your concern?
Done!
Done! |
|
@deiga 👋 Gentle nudge! Have I addressed your concerns in the response above? |
Per request of @deiga (integrations#2514 (comment)), this adds tests for previewHeaderInjectorTransport.RoundTrip logic around application/octest-stream header handling. See google/go-github#3392 for context. Signed-off-by: Mike Ball <mikedball@gmail.com>
d5e51a6 to
0cdd8e1
Compare
Per code review feedback from @deiga (integrations#2514 (comment)), this updates the github_release_asset data source tests to use the new testing patterns implemented in integrations#2986 This was tested via: ``` $ TF_ACC=1 \ go test -v ./... -run ^TestAccGithubReleaseAssetDataSource ? github.com/integrations/terraform-provider-github/v6 [no test files] === RUN TestAccGithubReleaseAssetDataSource === RUN TestAccGithubReleaseAssetDataSource/queries_specified_asset_ID --- PASS: TestAccGithubReleaseAssetDataSource (5.91s) --- PASS: TestAccGithubReleaseAssetDataSource/queries_specified_asset_ID (5.91s) PASS ok github.com/integrations/terraform-provider-github/v6/github 6.788s ``` Signed-off-by: Mike Ball <mikedball@gmail.com>
@deiga Thanks! I pushed a commit doing so. How's that look? |
Per request of @deiga (integrations#2514 (comment)), this adds tests for previewHeaderInjectorTransport.RoundTrip logic around application/octest-stream header handling. See google/go-github#3392 for context. Signed-off-by: Mike Ball <mikedball@gmail.com>
Per code review feedback from @deiga (integrations#2514 (comment)), this updates the github_release_asset data source tests to use the new testing patterns implemented in integrations#2986 This was tested via: ``` $ TF_ACC=1 \ go test -v ./... -run ^TestAccGithubReleaseAssetDataSource ? github.com/integrations/terraform-provider-github/v6 [no test files] === RUN TestAccGithubReleaseAssetDataSource === RUN TestAccGithubReleaseAssetDataSource/queries_specified_asset_ID --- PASS: TestAccGithubReleaseAssetDataSource (5.91s) --- PASS: TestAccGithubReleaseAssetDataSource/queries_specified_asset_ID (5.91s) PASS ok github.com/integrations/terraform-provider-github/v6/github 6.788s ``` Signed-off-by: Mike Ball <mikedball@gmail.com>
0cdd8e1 to
102ba5d
Compare
Per request of @deiga (integrations#2514 (comment)), this adds tests for previewHeaderInjectorTransport.RoundTrip logic around application/octest-stream header handling. See google/go-github#3392 for context. Signed-off-by: Mike Ball <mikedball@gmail.com>
Per code review feedback from @deiga (integrations#2514 (comment)), this updates the github_release_asset data source tests to use the new testing patterns implemented in integrations#2986 This was tested via: ``` $ TF_ACC=1 \ go test -v ./... -run ^TestAccGithubReleaseAssetDataSource ? github.com/integrations/terraform-provider-github/v6 [no test files] === RUN TestAccGithubReleaseAssetDataSource === RUN TestAccGithubReleaseAssetDataSource/queries_specified_asset_ID --- PASS: TestAccGithubReleaseAssetDataSource (5.91s) --- PASS: TestAccGithubReleaseAssetDataSource/queries_specified_asset_ID (5.91s) PASS ok github.com/integrations/terraform-provider-github/v6/github 6.788s ``` Signed-off-by: Mike Ball <mikedball@gmail.com>
102ba5d to
83ab3d7
Compare
deiga
left a comment
There was a problem hiding this comment.
LGTM! @stevehipwell what do you think about the test variable additions?
stevehipwell
left a comment
There was a problem hiding this comment.
Thanks for the PR @mdb, I've added some review comments.
Per request of @deiga (integrations#2514 (comment)), this adds tests for previewHeaderInjectorTransport.RoundTrip logic around application/octest-stream header handling. See google/go-github#3392 for context. Signed-off-by: Mike Ball <mikedball@gmail.com>
|
@mdb it looks like you've got failing tests. |
@stevehipwell Doh, thanks! I've pushed a fix. |
|
@mdb could you please rebase this so we can get it merged? |
This addresses issue integrations#2513 and adds support for a `github_release_asset` data source. Example of passing acceptance tests: ``` GITHUB_ORGANIZATION=mterwill \ GITHUB_OWNER=mterwill \ TF_ACC=1 \ go test -v ./... -run ^TestAccGithubReleaseAssetDataSource ? github.com/integrations/terraform-provider-github/v6 [no test files] === RUN TestAccGithubReleaseAssetDataSource === RUN TestAccGithubReleaseAssetDataSource/queries_specified_asset_ID === RUN TestAccGithubReleaseAssetDataSource/queries_specified_asset_ID/with_an_anonymous_account provider_utils.go:51: GITHUB_TOKEN environment variable should be empty provider_utils.go:74: Skipping TestAccGithubReleaseAssetDataSource/queries_specified_asset_ID/with_an_anonymous_account which requires anonymous mode === RUN TestAccGithubReleaseAssetDataSource/queries_specified_asset_ID/with_an_individual_account === RUN TestAccGithubReleaseAssetDataSource/queries_specified_asset_ID/with_an_organization_account --- PASS: TestAccGithubReleaseAssetDataSource (11.65s) --- PASS: TestAccGithubReleaseAssetDataSource/queries_specified_asset_ID (11.65s) --- SKIP: TestAccGithubReleaseAssetDataSource/queries_specified_asset_ID/with_an_anonymous_account (0.00s) --- PASS: TestAccGithubReleaseAssetDataSource/queries_specified_asset_ID/with_an_individual_account (8.90s) --- PASS: TestAccGithubReleaseAssetDataSource/queries_specified_asset_ID/with_an_organization_account (2.75s) PASS ok github.com/integrations/terraform-provider-github/v6/github 12.434s ``` Signed-off-by: Mike Ball <mikedball@gmail.com>
Per request of @deiga (integrations#2514 (comment)), this adds tests for previewHeaderInjectorTransport.RoundTrip logic around application/octest-stream header handling. See google/go-github#3392 for context. Signed-off-by: Mike Ball <mikedball@gmail.com>
Per code review feedback from @deiga (integrations#2514 (comment)), this updates the github_release_asset data source tests to use the new testing patterns implemented in integrations#2986 This was tested via: ``` $ TF_ACC=1 \ go test -v ./... -run ^TestAccGithubReleaseAssetDataSource ? github.com/integrations/terraform-provider-github/v6 [no test files] === RUN TestAccGithubReleaseAssetDataSource === RUN TestAccGithubReleaseAssetDataSource/queries_specified_asset_ID --- PASS: TestAccGithubReleaseAssetDataSource (5.91s) --- PASS: TestAccGithubReleaseAssetDataSource/queries_specified_asset_ID (5.91s) PASS ok github.com/integrations/terraform-provider-github/v6/github 6.788s ``` Signed-off-by: Mike Ball <mikedball@gmail.com>
…l pattern Per code review feedback (integrations#2514 (comment)) from @stevehipwell, this tweaks the `data_source_github_release_asset` to use the `if err := ...; err != nil` pattern towards the goal of improving readability. Signed-off-by: Mike Ball <mikedball@gmail.com>
Use idiomatic `:=` pattern over `var`-pre-defined variables, per code review feedback from @stevehipwell: integrations#2514 (comment) Signed-off-by: Mike Ball <mikedball@gmail.com>
…Func This inlines `resource.ComposeTestCheckFunc` to improve readability (rather than define it as a variable), per code review feedback from @stevehipwell: integrations#2514 (comment) Signed-off-by: Mike Ball <mikedball@gmail.com>
Per code review feedback from @stevehipwell, this simplifies test configuration: - source test configuration from `testAccConf`, as done elsewhere - remove the ability to override test configuration via `GH_TEST_*` env vars; this is arguably premature optimization and can be added to `acc_test.go` if it's needed in the future Signed-off-by: Mike Ball <mikedball@gmail.com>
…Providers Per code review feedback from @deiga, this removes unnecessary Providers configuration from the data_source_github_release_asset tests. integrations#2514 (comment) Signed-off-by: Mike Ball <mikedball@gmail.com>
To protect against the possibility that release asset IDs are not globally unique across GitHub, this configures the use of a 3-part composite ID when tracking `data_source_github_release_asset` instances in TF state. This addresses code review feedback from @deiga: integrations#2514 (comment) Signed-off-by: Mike Ball <mikedball@gmail.com>
Per code review feedback from @stevehipwell (integrations#2514 (comment)), this avoids the use of `client.Repositories.DownloadReleaseAsset` out of concern the function modifies the GitHub client state which could cause unexpected behaviour in Terraform. Signed-off-by: Mike Ball <mikedball@gmail.com>
This fixes a copy/paste bug; updated_at is now correctly set on the data source. Signed-off-by: Mike Ball <mikedball@gmail.com>
Per code review feedback from @stevehipwell (integrations#2514 (review)), this makes the release asset download optional, controlled by a `download_body` attribute, and off by default. Signed-off-by: Mike Ball <mikedball@gmail.com>
…ttributes Replace `download_body` argument and `body` attributes w/ `download_file`/`file`, respectively, towards the goal of improving the data source interface. This addresses code review feedback from @stevehipwell: integrations#2514 (comment) Signed-off-by: Mike Ball <mikedball@gmail.com>
Per code review feedback from @stevehipwell (integrations#2514 (comment)), this ensures `file` is base64-encoded. Signed-off-by: Mike Ball <mikedball@gmail.com>
Per code review feedback from @stevehipwell, this changes `download_file`/`file` to `download_file_contents`/`file_contents`, respectively: integrations#2514 (comment) Signed-off-by: Mike Ball <mikedball@gmail.com>
This improves the `data_source_github_release_asset` acceptance tests to be more TF-focused via use of the `base64decode` function, per code review request of @stevehipwell: integrations#2514 (comment) Signed-off-by: Mike Ball <mikedball@gmail.com>
This commits the result of `gofumpt`, as per lint failures in: https://github.com/integrations/terraform-provider-github/actions/runs/21232219897/job/61621068985?pr=2514 Signed-off-by: Mike Ball <mikedball@gmail.com>
This appeases lint warnings in `github/config_test.go`: https://github.com/integrations/terraform-provider-github/actions/runs/21232219897/job/61621068985?pr=2514 Signed-off-by: Mike Ball <mikedball@gmail.com>
This addresses the following staticcheck error (seen in https://github.com/integrations/terraform-provider-github/actions/runs/21232219897/job/61621068985?pr=2514): ``` Error: github/config.go:196:13: QF1001: could apply De Morgan's law (staticcheck) ``` Signed-off-by: Mike Ball <mikedball@gmail.com>
Go's http.Header canonicalizes header names (e.g., X-GitHub-Api-Version becomes X-Github-Api-Version). The test's unexpected header check was failing because it compared canonical names against non-canonical keys in the expectedHeaders map. Signed-off-by: Mike Ball <mikedball@gmail.com>
@stevehipwell Done! |
| return diag.FromErr(err) | ||
| } | ||
|
|
||
| req.Header.Set("Accept", "application/octet-stream") |
There was a problem hiding this comment.
Set and Accept header causes the http request to return 406 Not Acceptable.
req.Header.Set("Accept", "application/octet-stream")
| req.Header.Set("Accept", "application/octet-stream") |
| repository = "example-repository" | ||
| owner = "example-owner" | ||
| asset_id = 12345 | ||
| download_file = true |
There was a problem hiding this comment.
| download_file = true | |
| download_file_contents = true |
This addresses issue #2513 and adds support for a
github_release_assetdata source.Example of passing acceptance tests:
Updated testing, per feedback from @deiga :
Resolves #2513
Before the change?
github_release_assetdata sourceAfter the change?
github_release_assetdata sourcePull request checklist
Does this introduce a breaking change?
No.
Please see our docs on breaking changes to help!