-
Notifications
You must be signed in to change notification settings - Fork 162
Fix ruleset
for the v5 provider
#851
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
Conversation
@@ -480,6 +480,11 @@ func TestResourceGenerationV5(t *testing.T) { | |||
Transport: r, | |||
}, | |||
)) | |||
apiV0, _ = cfv0.New(viper.GetString("key"), viper.GetString("email"), cfv0.HTTPClient( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it looks like it assumes we're using API key + email for the old client. shouldn't it be initialising the same credentials we already have for the newer client?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's just for the unit tests though, which in the V4 test, only uses the key/email auth scheme.
On this commit, there's a fix for API client initializations to be the same outside of test runs: f4c7fcc
// The ruleset API has many gotchas that are accounted for in how we build | ||
// the 'response' object that feeds into the HCL generation, and it's difficult | ||
// to ensure the same compatability using the generated SDK. | ||
useOldSDK := resourceType == "cloudflare_ruleset" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it better copy the remapping logic instead of forcing this to use the old conditions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After a bit too much toiling, it appears so. Based on how we've built the V5 support in so far, I have to circumvent many assumptions we make about using the newer Go SDK. I thought this was the less bad option.
@musa-cf all the |
This uses the old SDK to assemble the combined rulesets and rules.
Closes #801