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

Remove use_account_from_zone for v2.0 refactor #468

Merged
merged 1 commit into from
Sep 17, 2019

Conversation

patryk
Copy link
Contributor

@patryk patryk commented Sep 16, 2019

Removes Account ID discovery logic, turning it into an explicit choice.
Part of breaking change track for v2.0.

@ghost ghost added size/S kind/documentation Categorizes issue or PR as related to documentation. labels Sep 16, 2019
@patryk patryk requested a review from jacobbednarz September 16, 2019 15:54
Copy link
Member

@jacobbednarz jacobbednarz left a comment

Choose a reason for hiding this comment

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

Woohoo! Pumped to see this go 🙂

@patryk patryk merged commit 9ff7ae9 into v2.0 Sep 17, 2019
@patryk
Copy link
Contributor Author

patryk commented Sep 17, 2019

Then it goes! :)

@patryk patryk deleted the use_account_from_zone-removal branch September 17, 2019 10:52
patryk pushed a commit that referenced this pull request Sep 30, 2019
* Update to cloudflare-go 0.10.0

Pulls in the deprecation of `Organization` in favour of `Account`.

Closes #227

* add go-version file

* newer go version

* Last org reference

* Rename "key" to "token"

API "token" is currently used for HTTP authentication requests where the
`X-Auth-Key` needs to be set. Until recently, it was fine to use the
wrong term for this authentication method (as there was only the one)
however there is now a feature from Cloudflare which is actually called
API tokens and it requires a different authentication procedure.

The functionality for API tokens has been released (v1.18.0) and this
cleans up the old references from "token" to "key" to better reflect
it's purpose.

* Remove use_account_from_zone for v2.0 refactor (#468)

* SDK-80: Zone to zone_id refactoring (#472)

* Refactor zone_id for resource_cloudflare_access_rule

* Refactor zone_id for cloudflare_filter

* Refactor zone_id for cloudflare_firewall_rule

* Refactor zone_id for cloudflare_load_balancer

* Refactor zone_id for cloudflare_page_rule

* Refactor zone_id for cloudflare_rate_limit

* Refactor zone_id for cloudflare_record

* Refactor zone_id for cloudflare_waf_rule

* Refactor zone_id for cloudflare_worker_route

* Refactor zone_id for cloudflare_worker_script

* Refactor zone_id for cloudflare_zone_lockdown

* Refactor zone_id for cloudflare_zone_settings_override

* Make tests `zone_id` compatible

Gets the build green by updating all tests to match the expected
behaviour of using zone ID instead of relying on zone from the
schema.

* Get rid of additional test output

* Add CLOUDFLARE_ALT_ZONE_ID to precheck

* Don't use API token authentication for Workers

The Workers service does not currently support the use of API tokens
(which is used by default when the `CLOUDFLARE_API_TOKEN` environment
variable is set[1]) and throws a very misleading exception.

```
=== RUN   TestAccCloudflareWorkerScript_SingleScriptNonEnt
--- FAIL: TestAccCloudflareWorkerScript_SingleScriptNonEnt (0.59s)
    testing.go:569: Step 0 error: errors during apply:

        Error: script already exists.

          on /var/folders/d4/5sgps61s2jg8f0_71663vw800000gn/T/tf-test617106443/main.tf line 2:
          (source code not available)
```

The reason we hit this exception is due to the `WorkerScript.Script`
being populated with the API error instead of being empty which falls
into this `if` block[2].

```
(cloudflare.WorkerScript) {
 WorkerMetaData: (cloudflare.WorkerMetaData) {
  ID: (string) "",
  ETAG: (string) "",
  Size: (int) 0,
  CreatedOn: (time.Time) 0001-01-01 00:00:00 +0000 UTC,
  ModifiedOn: (time.Time) 0001-01-01 00:00:00 +0000 UTC
 },
 Script: (string) (len=105) "{\"success\":false,\"errors\":[{\"code\":10000,\"message\":\"API Tokens are not supported by this API for now\"}]}\n"
}
```

This was initially puzzling as `err` is handled in `cloudflare-go` where
this field is defined[3] but looking deeper, the HTTP response is coming
back as a 200 OK response, hence the `Script` value being populated
and not falling into the `err` handling.

To fix this, it is two fold. The first is unsetting the
`CLOUDFLARE_API_TOKEN` environment variable in CI. This gets the tests
passing again however to address the underlying issue, we've contacted
Cloudflare to fix the HTTP response code which will then made everything
in the library work as intended. Depending on the length of time before
the fix is deployed, we might also hotfix the library to instead check
`r.Success = true` as well to fail in the correct spot instead of
allowing this to bubble up in unexpected ways.

[1]: https://git.io/Jes9l
[2]: https://git.io/Jes9A
[3]: https://git.io/Jes9h

* SDK-81: Refactor Workers resources to remove single-script code. (#478)

* SDK-81: Refactor Workers resources to remove single-script code.
Closes #470
Closes #387

* Version 2 Upgrade Guide (#480)

* Version 2 Upgrade Guide

* Update website/docs/guides/version-2-upgrade.html.markdown

Co-Authored-By: Jacob Bednarz <jacob.bednarz@gmail.com>

* Apply suggestions from code review

Co-Authored-By: Jacob Bednarz <jacob.bednarz@gmail.com>

* Drop go 1.11, add 1.13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/documentation Categorizes issue or PR as related to documentation. size/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants