-
Notifications
You must be signed in to change notification settings - Fork 24
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
feat(optimus): support multiple namespaces on replay and backup #63
Conversation
d79397c
to
8f4aa9a
Compare
…wnstream for replay
…ckup dry run response
…es to BackupResponse
8f4aa9a
to
e51a370
Compare
…am and allowed_downstream in replay and backup
e30ef93
to
5b28d20
Compare
odpf/optimus/runtime_service.proto
Outdated
@@ -773,11 +778,13 @@ message BackupDryRunRequest { | |||
string resource_name = 3; | |||
string namespace = 4; | |||
string description = 5; | |||
bool ignore_downstream = 6; | |||
bool ignore_downstream = 6 [deprecated=true]; | |||
repeated string allowed_downstream = 7; |
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.
what if we rename it to downstream_namespace_filter
? Also add some comments about how this work?
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.
sure i can add the comment. for the name, does the concern of naming it allowed_downstream
is because it is not clear enough that it is representing namespaces? if yes, does allowed_downstream_namespace
sounds better than downstream_namespace_filter
? to avoid questions on what is this filter, why it is needed.
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.
as discussed, will go with allowed_downstream_namespaces
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 @kushsharma , have changed to allowed_downstream_namespaces
and added the comments. please take a look.
LGTM |
No description provided.