-
Notifications
You must be signed in to change notification settings - Fork 859
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
Allow delete namespace by Id #2643
Allow delete namespace by Id #2643
Conversation
@@ -60,21 +65,25 @@ func NewActivities( | |||
} | |||
} | |||
|
|||
func (a *activities) GetNamespaceIDActivity(ctx context.Context, nsName namespace.Name) (namespace.ID, error) { | |||
func (a *activities) GetNamespaceInfoActivity(ctx context.Context, nsID namespace.ID, nsName namespace.Name) (getNamespaceInfoResult, 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.
why do we need to support by ID?
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 believe it can be useful for some recovery scenarios. Like rename went through but something failed and used doesn't know new name with random suffix.
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.
But how would they know what is their namespaceID? We would have to lookup their namespace ID by their old namespace name in the form of old_name.deleted.random. We can continue to use the renamed new name instead of namespace ID. If you look at persistence, namespaceID is used to lookup for namespace name and then from namespaces table by their name.
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.
NamespaceId
is not purely internal thing. It is, at least, in the logs and DescribeNamespace
response. And it stays unchanged during the process. I don't know exact scenario where it might be useful but my gut feelings tell me that it will be.
Also DescribeNamespace
can accept both name or Id, so from parity prospective it makes sense.
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.
Let's keep it as workflow param but not expose via API. If we found it useless I will remove it from workflow params too.
What changed?
Allow delete namespace by Id.
Why?
This was initial intent but for it was partially impemented in original PR.
How did you test it?
Added new unit tests.
Potential risks
No risks.
Is hotfix candidate?
No.