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

Resolve ignored errors in import cloudflare_access_application #3075

Merged
merged 3 commits into from
Jan 30, 2024

Conversation

daku10
Copy link
Contributor

@daku10 daku10 commented Jan 23, 2024

Firstly, thanks for the great work on the provider.

While working with the import cloudflare_access_application command, I encountered an issue where it returns a generic error message failed to read Access Application state. This error message is not very informative and obscures the underlying issue.

I propose a modification inspired by the approach already used. (e.g.

)

This PR is related to #2855(already closed).

Looking forward to feedback and suggestions for further improvements.

@daku10 daku10 requested a review from jacobbednarz as a code owner January 23, 2024 11:13
Copy link
Contributor

github-actions bot commented Jan 23, 2024

changelog detected ✅

@jacobbednarz
Copy link
Member

can you elaborate on what steps you took to get the error and what error you actually hit?

what is there and what you've proposed is almost 1:1 however the error message is missing the readErr being outputted. with this change, you still get the error but it's further down in the call tree.

@daku10
Copy link
Contributor Author

daku10 commented Jan 29, 2024

Thanks for the review.
I'll try to explain with the minimal code.

variable "api_token" {}
variable "account_id" {}

terraform {
  required_providers {
    cloudflare = {
      source  = "cloudflare/cloudflare"
      version = "4.22.0"
    }
  }
}

provider "cloudflare" {
  api_token = var.api_token
}

resource "cloudflare_access_application" "example" {
  account_id = var.account_id
}
$ terraform import cloudflare_access_application.example <account_id>/<application_id>

With insufficient API token permissions, the current provider outputs a generic error.

% terraform import cloudflare_access_application.example <account_id>/<application_id>
cloudflare_access_application.example: Importing from ID "<account_id>/<application_id>"...
╷
│  Error: failed to read Access Application state
│
│
╵

With my proposed change, the output will be like this.

% terraform import cloudflare_access_application.example <account_id>/<application_id>
cloudflare_access_application.example: Importing from ID "<account_id>/<application_id>"...
cloudflare_access_application.example: Import prepared!
  Prepared cloudflare_access_application for import
cloudflare_access_application.example: Refreshing state... [id=<application_id>]
╷
│ Error: error finding Access Application "<application_id>": error from makeRequest: Authentication error (10000)
│
│
╵

Therefore I can know the error is caused by the permission of the API token. I mimicked the behavior of the cloudflare_workers_script resource.

I observed that in the current provider, error handling varies - some resources ignore errors while others don't.

Errors are ignored:

Errors are not ignored:

Given this inconsistency, I'm proposing a similar approach to what's been used in resources that ignore errors.

I'm aware this approach isn't ideal as it involves ignoring errors during the import phase and relying on subsequent read operations for error handling. I'd appreciate your thoughts on whether aligning with the existing pattern is preferable or if a different approach should be considered.

Thanks again for your time.

@jacobbednarz
Copy link
Member

i think this is fine for the use case here. in general, we want to perform existence checks before we run the Read operation to save state issues and additional HTTP calls however, we don't need that to be super strict here.

.changelog/3075.txt Outdated Show resolved Hide resolved
@jacobbednarz jacobbednarz merged commit e67c75c into cloudflare:master Jan 30, 2024
8 checks passed
@github-actions github-actions bot added this to the v4.24.0 milestone Jan 30, 2024
github-actions bot pushed a commit that referenced this pull request Jan 30, 2024
Copy link
Contributor

github-actions bot commented Feb 7, 2024

This functionality has been released in v4.24.0 of the Terraform Cloudflare Provider.

Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants