Skip to content
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

Add docker compose restart #316

Merged
merged 6 commits into from Jul 20, 2018
Merged

Add docker compose restart #316

merged 6 commits into from Jul 20, 2018

Conversation

PrashanthCorp
Copy link
Contributor

Fixes #250.
Addresses #73.

@PrashanthCorp
Copy link
Contributor Author

The build failed due to a timeout. Restarting build

@@ -100,3 +100,8 @@ export function composeUp(dockerComposeFileUri?: vscode.Uri, selectedComposeFile
export function composeDown(dockerComposeFileUri?: vscode.Uri, selectedComposeFileUris?: vscode.Uri[]) {
compose('down', 'to take down', dockerComposeFileUri, selectedComposeFileUris);
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you tried running this from the command palette? I think you'll find something undesirable happen.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I don't know of any other way to run docker compose Up or Down (the package.json only specifies them in the commands section, not in any context menu).

The undesirable side-effect is that I'm asked to choose a compose file once for each of the compose down and up operations.
@fiveisprime, is that a critical point of friction?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't you be annoyed? It's just code, you can rewrite it to suit the necessary experience.

@StephenWeatherford
Copy link
Contributor

Also, I don't see any CTI tests... Please add them for compose up/down as well and anything related.

command: teleCmdId + command
for (let command of commands) {
selectedItems.forEach((item: Item) => {
terminal.sendText(command.toLowerCase() === 'up' ? `docker-compose -f ${item.file} ${command} ${detached} ${build}` : `docker-compose -f ${item.file} ${command}`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you move the ? : clauses onto separate lines for better readability? Thx.

@@ -34,7 +34,7 @@ function computeItems(folder: vscode.WorkspaceFolder, uris: vscode.Uri[]): vscod
return items;
}

async function compose(command: string, message: string, dockerComposeFileUri?: vscode.Uri, selectedComposeFileUris?: vscode.Uri[]): Promise<void> {
async function compose(commands: string[], message: string, dockerComposeFileUri?: vscode.Uri, selectedComposeFileUris?: vscode.Uri[]): Promise<void> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

commands: ('up' | 'down')[]

reporter.sendTelemetryEvent('command', {
command: teleCmdId + command
for (let command of commands) {
selectedItems.forEach((item: Item) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I know, not your code, but... personally I don't like using forEach because you can't just debug through the loop with F10, you have to set a breakpoint in the function. for of is so much cleaner now.

@PrashanthCorp PrashanthCorp merged commit 4741f0e into master Jul 20, 2018
@PrashanthCorp PrashanthCorp deleted the ps/ComposeRestart branch September 6, 2018 23:27
@microsoft microsoft locked and limited conversation to collaborators Oct 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants