-
Notifications
You must be signed in to change notification settings - Fork 937
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
Remove migrated cluster handlers #7446
base: main
Are you sure you want to change the base?
Conversation
@@ -1117,40 +1116,8 @@ func (adh *AdminHandler) DescribeCluster( | |||
}, nil | |||
} | |||
|
|||
// ListClusters return information about temporal clusters | |||
// TODO: Remove this API after migrate tctl to use operator handler |
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.
TCTL is deprecated, and CLI doesn't use admin service at all.
From Slack:
The year or two ago they made the temporal CLI, they decided not to port admin things and only access frontend. I think their reasoning was that the server or tdbg or some other CLI could do it, unsure
|
||
// ListClusterMembers | ||
// TODO: Remove this API after migrate tctl to use operator handler |
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 comment was made somewhat in error. This api is still critical, not duplicated anywhere and still used by tdbg.
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.
cc @cgs
} | ||
|
||
message ListClustersResponse { | ||
repeated temporal.server.api.persistence.v1.ClusterMetadata clusters = 1; |
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.
operator service seem to return a different type here?
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 this is ok. But we should try to migrate use cases to operator service before merge this.
LGTM. I have some internal use cases but we can migrate to operator handler |
What changed?
Removed Handlers in admin service that have finished migrating.
Why?
Dropping duplicated code.
How did you test it?
Testing coverage, reviewed dependencies.
Potential risks
Little to none, admin service is considered internal only.
Documentation
N/A
Is hotfix candidate?
No