Secrets: Error standardization - Secrets API #7691
Conversation
Signed-off-by: Pavlo Glushko <[email protected]>
pkg/api/errors/secrets.go
Outdated
| codes.PermissionDenied, | ||
| http.StatusForbidden, | ||
| fmt.Sprintf("access denied by policy to get %q from %q", key, s.name), | ||
| "ERR_SECRETS_PERMISSION_DENIED", |
There was a problem hiding this comment.
| "ERR_SECRETS_PERMISSION_DENIED", | |
| "ERR_PERMISSION_DENIED", |
We have to keep this as the legacy tag for backwards compatibility
pkg/api/errors/secrets.go
Outdated
| fmt.Sprintf("failed getting secret with key %s from secret store %s: %s", key, s.name, error), | ||
| "ERR_SECRET_GET", | ||
| ), | ||
| "SECRET_GET", |
There was a problem hiding this comment.
| "SECRET_GET", | |
| "GET_SECRET", |
Since we prefix the reason with the errors.CodePrefixSecretStore in the build(), it might be worth changing it to SECRET_GET_SECRET - then its roughly api_operation_thing.
There was a problem hiding this comment.
Makes sense
Should tags be updated accordingly? ERR_SECRET_GET => ERR_GET_SECRET (for other scenarios where applicable also)
There was a problem hiding this comment.
"ERR_SECRET_GET" will have to remain as is since its the legacy tag. The new tag (or errCode) can be updated accordingly, as you have correctly already 🙂
pkg/api/errors/secrets.go
Outdated
| fmt.Sprintf("failed getting secrets from secret store %s: %v", s.name, error), | ||
| "ERR_SECRET_GET", | ||
| ), | ||
| "SECRET_GET", |
There was a problem hiding this comment.
| "SECRET_GET", | |
| "GET_BULK_SECRET", |
There was a problem hiding this comment.
Great start @pglushko - thanks for keeping the error codes the same for backwards compatibility 🙂 Next will be writing the integration tests. I don't see a secrets folder in the integration/framework folder, so that will be the next step to add in order to write the tests for the errors. Lmk if you need any assistance 🙂
| @@ -75,13 +75,6 @@ var ( | |||
| ErrOutboundHealthNotReady = APIError{"dapr outbound is not ready", "ERR_OUTBOUND_HEALTH_NOT_READY", http.StatusInternalServerError, grpcCodes.Internal} | |||
| ErrHealthAppIDNotMatch = APIError{"dapr app-id does not match", "ERR_HEALTH_APPID_NOT_MATCH", http.StatusInternalServerError, grpcCodes.Internal} | |||
|
|
|||
| // Secrets. | |||
| ErrSecretStoreNotConfigured = APIError{"secret store is not configured", "ERR_SECRET_STORES_NOT_CONFIGURED", http.StatusInternalServerError, grpcCodes.FailedPrecondition} | |||
There was a problem hiding this comment.
@cicoyle looks like typo here btw. ERR_SECRET_STORES_NOT_CONFIGURED => ERR_SECRET_STORE_NOT_CONFIGURED. Fix it?
There was a problem hiding this comment.
We will have to keep the legacy tag as is: ERR_SECRET_STORES_NOT_CONFIGURED, the new tag that gets the api prefixed in the build() can be whatever makes the most sense.
I took a glance at integration tests and I would definitively need some guidance there. How should I tackle it? |
Signed-off-by: Pavlo Glushko <[email protected]>
The integration tests will involve writing a new secret |
Description
Issue reference
#7690
#7484
Please reference the issue this PR will close: #7690
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: