-
Notifications
You must be signed in to change notification settings - Fork 85
feat(shell-api): add sp.listWorkspaceDefaults command MONGOSH-2949 #2558
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
packages/shell-api/src/streams.ts
Outdated
| } | ||
|
|
||
| @returnsPromise | ||
| async listWorkspaceDefaults() { |
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.
Can we add a type annotation to this method? I know we're super inconsistent about it, but we'd probably want to move in a direction where we more strictly specify the types and it would be nice if we already do that for all new methods.
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.
Done! Not sure if what I did is overboard, I used some other examples I saw. Happy to adjust @gagik
12abb96 to
c7fb5c6
Compare
packages/shell-api/src/streams.ts
Outdated
| } | ||
|
|
||
| @returnsPromise | ||
| async listWorkspaceDefaults(): Promise<WorkspaceDefaults | Document> { |
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.
| Document basically means this can also return any other arbitrary object. Is that the case? i.e. could the data be different based on version perhaps? It's fine to be kept like this if we don't have guarantees around it. Which with a database call we generally don't.
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 is unneeded now. I originally was using this in the case we had an error. But Anna pointed out the code is unreachable so I was able to simplify this, removed Document
packages/shell-api/src/streams.ts
Outdated
| const result = await this._runStreamCommand({ | ||
| listWorkspaceDefaults: 1, | ||
| }); | ||
| if (result.ok !== 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.
This should be unreachable – ok: 0 should result in a driver-level exception
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.
Ah thank you, I wasn't sure about this since we were checking above. Simplified the implementation here
|
just pushed up a fix so ready for another look, attached a screenshot as well showing the command is working with the fieldname change |
Ticket: MONGOSH-2949
Description:
Adds a new sp.listWorkspaceDefaults command to return the (default) tier and maxTier size of the stream processing workspace attached to a stream processor