Conversation
|
|
||
| export class ReplicaListStep extends AzureWizardPromptStep<IStreamLogsContext> { | ||
| public async prompt(context: IStreamLogsContext): Promise<void> { | ||
| const placeHolder: string = localize('selectReplica', 'Select a Replica'); |
There was a problem hiding this comment.
nit: lowercase replica. We typically only capitalize things that have Azure branding in it
| if (!logStream) { | ||
| const allStreams = getActiveLogStreams(context); | ||
| for (const streams of allStreams.values()) { | ||
| if (streams && streams.isConnected) { |
There was a problem hiding this comment.
I think the cleaner way to do this is to make this block of code a utility method and then just calling it on logStream or in each stream in allStreams.
You could also do something like
const allStreams = context.logStreamToStop ? [context.logStreamToStop] : getActiveLogStreams(context);
| export async function disconnectLogStreaming(context: IStreamLogsContext): Promise<void> { | ||
| const allStreams = context.logStreamToStop ? [context.logStreamToStop] : getActiveLogStreams(context); | ||
|
|
||
| for (const streams of allStreams.values()) { |
There was a problem hiding this comment.
What happens if there are no active log streams? We should probably throw an error. There's another spot getActiveLogStreams is used (and an error is thrown) so maybe it makes sense to just have the error be thrown from that function?
There was a problem hiding this comment.
StreamListStep currently throws an error if there are no active streams. This is called before disconnectLogStreaming so it should already be thrown if there are no active streams.
There was a problem hiding this comment.
That makes sense.
I'd personally still prefer to have the error be thrown in getActiveLogStreams though. I know it's probably unlikely, but in the event that we ever have to call disconnectLogStreaming from somewhere else, it'd be good to have the error behavior consistent.
| const placeHolder: string = localize('selectRevision', 'Select a revision'); | ||
| context.revision = (await context.ui.showQuickPick(this.getPicks(context), { placeHolder })).data; | ||
| } else { | ||
| (await this.getPicks(context)).forEach(revision => { |
There was a problem hiding this comment.
You can do this to get the latest/active revision. Single revision's latest one should always be the active, AFAIK.
const revisionData = await client.containerAppsRevisions.getRevision(this.resourceGroup, this.name, nonNullProp(this.containerApp, 'latestRevisionName'));
| } | ||
| } | ||
|
|
||
| public shouldPrompt(context: IStreamLogsContext): boolean { |
There was a problem hiding this comment.
I think you could use the configureBeforePrompt here effectively.
If the revision mode is single, then just set context.revision to the latest.
If the revision mode is multiple, but there is only 1 revision, then we can automatically set context.revision to that one.
| return !context.logStreamToStop; | ||
| } | ||
|
|
||
| private async getPicks(context: IStreamLogsContext): Promise<IAzureQuickPickItem<ILogStream>[]> { |
There was a problem hiding this comment.
I'm pretty sure that this doesn't need to be async.
| } | ||
|
|
||
| public async configureBeforePrompt(context: IStreamLogsContext): Promise<void> { | ||
| const picks = await this.getPicks(context); |
There was a problem hiding this comment.
We don't want to call this first. In the event that this is single revision mode, this call was pointless and makes the wizard slightly more sluggish.
I'd suggestion that you first check if the revisionsMode is single, then in your else clause, do the logic to see how many revisions there are.
There was a problem hiding this comment.
You may also want to cache the results of this.getPicks in the wizard step, otherwise we will call it again needlessly if the event we actually do prompt the users.
Since it's pretty rare to actually have multiple revision mode, I wouldn't mind too much if you don't cache it, but I thought I'd mention it for best practice :)
| public async prompt(context: IStreamLogsContext): Promise<void> { | ||
| const placeHolder: string = localize('selectRevision', 'Select a revision'); | ||
| context.revision = (await context.ui.showQuickPick(this.getPicks(context), { placeHolder })).data; | ||
| context.revision = (await context.ui.showQuickPick(await this.getPicks(context), { placeHolder })).data; |
There was a problem hiding this comment.
Generally you don't want to await this.getPicks in showQuickPick. showQuickPick is designed to handle promises so that it will show a loading bar while the promise resolves. If you await it, there'll be a small gap where the quickpick UI doesn't show up while it's loading the picks.
There was a problem hiding this comment.
whoops accidently added that ahaha.
| const placeHolder: string = localize('selectStream', 'Select a stream'); | ||
| const picks: IAzureQuickPickItem<ILogStream | undefined>[] = this.getPicks(context); | ||
| if (picks.length > 1) { | ||
| picks.push({ label: localize('stopAll', 'Stop all Streams'), data: undefined }); |
There was a problem hiding this comment.
lol.... nit: I'm not sure if it should just be Stop all streams since it's not exactly a title.
3 participants
Fixes #34.
When clicking
Stop Streaming Logs...users are prompted to choose which stream they would like to stop.There is a pick option to stop all streams. I thought it could be helpful to have a warning message pop up to confirm they want to stop all logs since it will immediately stop all logs after this step. Would that be something we would want to add?