-
-
Notifications
You must be signed in to change notification settings - Fork 128
Conversation
54156fe
to
a27e44a
Compare
a27e44a
to
bb1c40a
Compare
management/hook_secrets.go
Outdated
|
||
func newHookSecretsManager(m *Management) *HookSecretsManager { | ||
return &HookSecretsManager{m} | ||
} |
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 should add this to the HookManager
instead. See the UserManager
and RoleManager
for examples.
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 see your point. I'll move it into the hook manager.
management/hook_secrets.go
Outdated
// See: https://auth0.com/docs/api/management/v2#!/Hooks/post_secrets | ||
func (m *HookSecretsManager) Upsert(hookId string, r *HookSecrets) (err error) { | ||
return m.post(m.hookPath(hookId), r) | ||
} |
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.
POST /api/v2/hooks/{id}/secrets
doesn't have upsert semantics. Posting a map that contains an existing field will result in a conflict.
{
"statusCode": 409,
"error": "Conflict",
"message": "Error: Secret exists",
"errorCode": "SecretExists"
}
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.
OMG It's even worse than that. The PATCH doesn't work if you specify a new secret field. I found the only way to add a new secret to a hook that already has a secret is to completely DELETE all secrets for that hook then "POST" with all keys and values (even though you can't retrieve the values via the API). I've got a ticket open with Auth0 support about this.
management/hook_secrets.go
Outdated
// execution (they all appear as "_VALUE_NOT_SHOWN_") | ||
// | ||
// See: https://auth0.com/docs/api/management/v2#!/Rules_Configs/get_rules_configs | ||
func (m *HookSecretsManager) Read(hookId string) (r *HookSecrets, err error) { |
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.
When moved to the HookManager
this could be called Secrets
.
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.
Done
management/hook_secrets.go
Outdated
// Delete a list of hook secret keys from a given hook's secrets identified by its hookId and the keys | ||
// | ||
// See: https://auth0.com/docs/api/management/v2#!/Rules_Configs/delete_rules_configs_by_key | ||
func (m *HookSecretsManager) Delete(hookId string, keys ...string) (err error) { |
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.
Perhaps we can add a Keys() []string
method to HookSecrets
to help with specifying keys?
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.
Done
management/hook_secrets.go
Outdated
|
||
func (m *HookSecretsManager) hookPath(hookId string) string { | ||
return m.uri("hooks", hookId, "secrets") | ||
} |
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'm being a little pedantic here but this is not the hooks path, it's the hooks secrets path. I'd also remove it altogether.
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 get it. I hate duplicating code but I see how it's kinda already copied and pasted everywhere already. I'll remove it.
management/hook_secrets_test.go
Outdated
t.Fatal(err) | ||
} | ||
|
||
t.Cleanup(func() { |
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 was not aware of this function! Thanks! TIL.
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 wasn't aware of it until my IDE told me it existed :)
34ad06d
to
a9672b0
Compare
a9672b0
to
f78a5b7
Compare
if len(k) > 0 { | ||
keys[i] = k | ||
} | ||
i++ |
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.
This nil check has the potential to leave the keys
slice with less elements than it was made to hold right? Why must we check for len(k) > 0
?
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.
This is the only question remaining from my side, otherwise this PR is good to go
Thanks Matt! |
Mind making a tagged release after? :)