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

fix: make sure the authorization error is correctly shown to the end user #200

Merged

Conversation

Nkmol
Copy link

@Nkmol Nkmol commented Apr 1, 2024

This PR makes sure that an authorization error (401 Unauthorized) is at least shown to the end user.


Bitbucket responses, mainly headers and error codes, seem to have completely changed. For the unauthorized response, the Content-Type was changed to "text/plain" where no HTTP-body is returned any more.

Because of this, the client's decoding fails (see https://github.com/DrFaust92/bitbucket-go-client/blob/main/client.go#L387), and the original Bitbucket API error is no longer passed to the Terraform code.

To overcome this issue, I now rely on the raw HTTP Response and always log the original HTTP Code. The main files to review are the tests and the bitbucket/error.go file.

Authorization error

╷
│ Error: 401 Unauthorized: 
│ 
│   with bitbucket_repository.that,
│   on example.tf line 13, in resource "bitbucket_repository" "that":
│   13: resource "bitbucket_repository" "that" {
│ 

Other Bitbucket API error

╷
│ Error: 400 Bad Request: Repository with this Slug and Owner already exists.
│ 
│   with bitbucket_repository.that,
│   on example.tf line 13, in resource "bitbucket_repository" "that":
│   13: resource "bitbucket_repository" "that" {
│ 
╵

I ran the acceptance tests, but found some issues that also happen on the master branch.

An individual run, for example for the Bitbucket Repository resource, shows an error that is not caused by this PR:

?       github.com/terraform-providers/terraform-provider-bitbucket     [no test files]
=== RUN   TestAccBitbucketRepositoryGroupPermission_basic
--- PASS: TestAccBitbucketRepositoryGroupPermission_basic (19.98s)
=== RUN   TestAccBitbucketRepository_basic
--- PASS: TestAccBitbucketRepository_basic (9.63s)
=== RUN   TestAccBitbucketRepository_project
--- PASS: TestAccBitbucketRepository_project (10.74s)
=== RUN   TestAccBitbucketRepository_avatar
--- PASS: TestAccBitbucketRepository_avatar (9.26s)
=== RUN   TestAccBitbucketRepository_slug
--- PASS: TestAccBitbucketRepository_slug (8.16s)
=== RUN   TestAccBitbucketRepository_inherit
--- PASS: TestAccBitbucketRepository_inherit (22.73s)
=== RUN   TestAccBitbucketRepository_wrongCredential
--- PASS: TestAccBitbucketRepository_wrongCredential (0.71s)
=== RUN   TestAccBitbucketRepository_duplicate
--- PASS: TestAccBitbucketRepository_duplicate (5.49s)
=== RUN   TestAccBitbucketRepositoryUserPermission_basic
--- FAIL: TestAccBitbucketRepositoryUserPermission_basic (7.14s)
=== RUN   TestAccBitbucketRepositoryVariable_basic
--- PASS: TestAccBitbucketRepositoryVariable_basic (15.43s)
FAIL
FAIL    github.com/terraform-providers/terraform-provider-bitbucket/bitbucket   109.292s
FAIL

Manual tested a correct apply, authorization error and a Bitbucket API error.

@act-mreeves
Copy link

This looks useful. I've observed errors can be very opaque so bubbling it up from the bitbucket API seems helpful. #199

Example when I intentionally make my username (via BITBUCKET_USERNAME) incorrect.

╷
│ Error: Empty Summary: This is always a bug in the provider and should be reported to the provider developers.
│ 
│   with bitbucket_repository.foobar,
│   on foobar.tf line 1, in resource "bitbucket_repository" "foobar":
│    1: resource "bitbucket_repository" "foobar" {

Copy link
Owner

@DrFaust92 DrFaust92 left a comment

Choose a reason for hiding this comment

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

LGTM

@DrFaust92 DrFaust92 merged commit 902f7b3 into DrFaust92:master Apr 28, 2024
1 check passed
@Nkmol Nkmol deleted the fix-authorization-diagnostic-error branch April 29, 2024 22:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants