-
Notifications
You must be signed in to change notification settings - Fork 6
Fix the http paths for the validate endpoints #95
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
| rpc ValidateAccountAuditLogSink(ValidateAccountAuditLogSinkRequest) returns (ValidateAccountAuditLogSinkResponse) { | ||
| option (google.api.http) = { | ||
| post: "/cloud/account/audit-logs/sink/validate", | ||
| post: "/cloud/audit-log-sink/validate", |
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.
should this be "/cloud/account/audit-log-sink-validate"
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.
We decided to move audit logs into the root level.
There is no reason the audit logs sinks should be scoped under the account. All resources like namepspaces, apikeys, users etc are scoped to the account but the paths do not have a /account. We felt audit logs sinks should follow the same pattern.
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.
makes sense, so all the paths in this PR should be updated https://github.com/temporalio/cloud-api/pull/94/files
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.
Yes.
| rpc ValidateNamespaceExportSink(ValidateNamespaceExportSinkRequest) returns (ValidateNamespaceExportSinkResponse) { | ||
| option (google.api.http) = { | ||
| post: "/cloud/namespaces/{namespace}/export-sinks/validate", | ||
| post: "/cloud/namespaces/{namespace}/export-sink-validate", |
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.
QQ: validate is the function name? So we don't need /validate?
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.
The problem with "/cloud/namespaces/{namespace}/export-sinks/validate" is that it collides with an export sink with name "validate". This change is to avoid the collision.
No description provided.