-
-
Notifications
You must be signed in to change notification settings - Fork 151
Conversation
Hi @kevinschoonover, most of the errors I can account for, except for
|
@alexkappa just ran the tests again fixing the connection and think everything is good. output
|
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.
Hi @kevinschoonover, thanks for giving this a go! I've made some suggestions for your consideration 🙏
auth0/provider.go
Outdated
Required: true, | ||
DefaultFunc: schema.EnvDefaultFunc("AUTH0_CLIENT_ID", nil), | ||
Optional: true, | ||
DefaultFunc: schema.EnvDefaultFunc(AUTH0_CLIENT_ID, nil), |
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.
Do you think we can work with RequiredWith
or ConflictsWith
to handle us requiring either a client id/secret or a token?
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.
@alexkappa should be all done! let me know what your thoughts are about the error messages |
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.
Hey @kevinschoonover , thanks for submitting this! Despite my volume of feedback, I'm actually very in favor of your proposed changes and would like to work with you to craft this solution a bit further.
Aside from a few minor code comments, I have two concerns:
- Documentation: Because there was only one method of authentication before, it was clear what the required arguments are. With this change, we need to be a bit clearer in the docs about the potential authentication strategies a dev could use.
-Testing: We can manually verify that the declarative rules in the schema work, but I'd like us to consider applying a basic test to ensure that these rules don't change from under us.
One thing to note is that this project is migrating over to the Auth0 organization soon, and there is no way to migrate PRs. So we will either need to close and re-open this PR or merge before then.
@willvedd I've been digging into adding tests and running into a lot of trouble. Using the following code > cat provider_test.go
...
func TestProvider_configValidation(t *testing.T) {
testCases := []struct {
name string
environment map[string]string
expectedErrors []error
}{
{
name: "empty",
environment: map[string]string{"AUTH0_DOMAIN": "TEST", "AUTH0_CLIENT_ID": "test", "AUTH0_CLIENT_SECRET": "test2", "AUTH0_TOKEN": "TEST"},
expectedErrors: []error{},
},
}
for _, test := range testCases {
t.Run(test.name, func(t *testing.T) {
for k, v := range test.environment {
fmt.Println(k, v)
os.Unsetenv(k)
os.Setenv(k, v)
}
c := terraform.NewResourceConfigRaw(nil)
p := Provider()
r, errs := p.Validate(c)
assert.Equal(t, test.expectedErrors, errs)
fmt.Println(r, errs)
for k := range test.environment {
os.Unsetenv(k)
}
})
}
} I keep getting the following error: > go test -test.run TestProvider_configValidation -test.v
...
provider_test.go:107:
Error Trace: provider_test.go:107
Error: Not equal:
expected: []error{}
actual : []error{(*errors.errorString)(0xc0005c49e0), (*errors.errorString)(0xc0005c4af0)}
Diff:
--- Expected
+++ Actual
@@ -1,2 +1,4 @@
-([]error) {
+([]error) (len=2) {
+ (*errors.errorString)("client_id": all of `client_id,client_secret` must be specified),
+ (*errors.errorString)("client_secret": all of `client_id,client_secret` must be specified)
}
Test: TestProvider_configValidation/empty
... Reading into the terraform-plugin-sdk, I think this has something to do with the value 'client_id' and 'client_secret' not being concurrently populated from the environment using the 'DefaultFunc' so it thinks the other one does not exist when it calls the validateRequiredWithAttribute function. I'm not really sure why this works running it via CLI though. As a temporary workaround, I can use NewResourceConfigRaw to test the schema validation; however, it won't cover the normal case where users will pass these in as environment variables. Let me know your thoughts (and if this explanation makes sense) |
@willvedd updated the tests, let me know your thoughts. Happy to change anything about them! |
dfc1178
to
8d18b59
Compare
@kevinschoonover Thanks for making those changes, it's really coming together. Those tests are what I had in mind, I'll just need to pull down and run them locally. But as far as feedback goes, I don't have anything major. Because this is a change in the fundamental operation of the provider, I'd like @sergiughf 's approval too. |
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.
Great contribution @kevinschoonover! Only left a few comments I'd be happy for us to discuss.
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.
Thanks a lot for the super quick follow up on our review! Once we tackle the last comments we're ready to merge.
Co-authored-by: Sergiu Ghitea <28300158+sergiughf@users.noreply.github.com>
Co-authored-by: Sergiu Ghitea <28300158+sergiughf@users.noreply.github.com>
Co-authored-by: Sergiu Ghitea <28300158+sergiughf@users.noreply.github.com>
@sergiughf Comments should now be addressed! Let me know if there is anything else! |
auth0/provider_test.go
Outdated
for k, v := range test.environment { | ||
os.Unsetenv(k) | ||
os.Setenv(k, v) | ||
} |
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.
I ran the full test suite locally and unfortunately having both resourceConfig set up and the env vars messes up with running everything in 1 go as the env vars will be unset for the subsequent acceptance tests. To fix this we could either:
A) keep track of env vars before setting the ones in the tests and replace the original values after they run
B) test only with the resourceConfig values
Apologies for having missed this before @kevinschoonover. Would you be able to remove the env vars from these tests and just keep the resourceConfig as that should already cover our intended behavior change in the auth mechanism?
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.
I think we need to do A) because I ran the acceptance test again removing this code and its pulling client_id and client_secret from the environment breaking the existing tests.
> AUTH0_DOMAIN="https://<tenant>.auth0.com" AUTH0_CLIENT_ID="<id>" AUTH0_CLIENT_SECRET="<id>" make testacc
=== RUN TestProvider_configValidation
=== RUN TestProvider_configValidation/missing_client_id
provider_test.go:134: actual did not match expected. len(expected) != len(actual). expected: ["client_secret": all of `client_id,client_secret` must be specified], actual: ["client_secret": all of `client_id,client_secret` must be specified "client_id": all of `client_id,client_secret` must be specified]
=== RUN TestProvider_configValidation/missing_client_secret
provider_test.go:134: actual did not match expected. len(expected) != len(actual). expected: ["client_id": all of `client_id,client_secret` must be specified], actual: ["client_id": all of `client_id,client_secret` must be specified "client_secret": all of `client_id,client_secret` must be specified]
=== RUN TestProvider_configValidation/conflicting_auth0_client_and_management_token_without_domain
provider_test.go:134: actual did not match expected. len(expected) != len(actual). expected: ["domain": required field is not set "client_id": conflicts with api_token "client_secret": conflicts with api_token "api_token": conflicts with client_id], actual: ["api_token": conflicts with client_id "client_id": conflicts with api_token "client_secret": conflicts with api_token]
=== RUN TestProvider_configValidation/valid_auth0_client
=== RUN TestProvider_configValidation/valid_auth0_token
provider_test.go:134: actual did not match expected. len(expected) != len(actual). expected: [], actual: ["client_secret": all of `client_id,client_secret` must be specified "client_id": all of `client_id,client_secret` must be specified]
--- FAIL: TestProvider_configValidation (0.06s)
--- FAIL: TestProvider_configValidation/missing_client_id (0.01s)
--- FAIL: TestProvider_configValidation/missing_client_secret (0.01s)
--- FAIL: TestProvider_configValidation/conflicting_auth0_client_and_management_token_without_domain (0.01s)
--- PASS: TestProvider_configValidation/valid_auth0_client (0.01s)
--- FAIL: TestProvider_configValidation/valid_auth0_token (0.01s)
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.
Am able to confirm that all acceptance tests pass and the TestProvider_configValidation
unit test passes reliably. So with that I say, great work! And thank you for your patience.
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.
Great work @kevinschoonover !
Proposed Changes
Acceptance Test Output
I ran the acceptance tests using
AUTH0_MANAGEMENT_TOKENS
tokens so I'm not sure if the test failures are flaky tests, problems with using a management token, or expected behavior.output
Community Note