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 a setting to control the compose command used #3478

Merged
merged 6 commits into from
Mar 31, 2022
Merged

Conversation

bwateratmsft
Copy link
Collaborator

@bwateratmsft bwateratmsft commented Mar 30, 2022

Fixes #2977. A string setting, docker.composeCommand is added. If unset, the extension will try to use docker compose by running docker compose version; if it succeeds we assume we can use docker compose, otherwise we use docker-compose. If the context or settings are altered, we will re-run that check, since different contexts have different requirements (for example, ACI contexts require docker compose, not supporting docker-compose at all).

I expect few people will need to force docker-compose when docker compose does work, but for them, they can use the setting in either workspace or user settings.

I marked #2977 as having documentation impact, because it will require documenting the new ${composeCommand} placeholder in command customization.

Scenarios to test:

  • Login/logout
  • Push/pull image
  • Azure CLI
  • Deploy to ACI
  • View ACI containers
  • .NET debug
  • Python debug
  • Container files
    • Windows
    • Linux
  • docker-build task
  • docker-run task
  • docker-compose task
  • Compose group commands
  • Custom commands
  • Fallback on systems not supporting docker compose

@@ -1604,7 +1604,7 @@
"type": "string"
}
],
"default": "docker build --pull --rm -f \"${dockerfile}\" -t ${tag} \"${context}\"",
"default": "${config:docker.dockerPath} build --pull --rm -f \"${dockerfile}\" -t ${tag} \"${context}\"",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Rather than always running docker ..., we can pick up the setting they apply with docker.dockerPath. In contrast to docker.composeCommand, that setting is not unset by default, because we aren't attempting to do any autodetection.

@@ -1932,14 +1932,7 @@
"default": [
{
"label": "Compose Down",
"template": "docker-compose ${configurationFile} down",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since we now intelligently choose the compose command, we no longer need separate command templates based on context for compose down. It is still needed for the two compose up commands, because ACI contexts lack a --build flag and will error if it is present.

@@ -51,7 +50,6 @@ export abstract class DockerTaskProvider implements TaskProvider {
context.platform = getPlatform(task.definition);

context.actionContext.telemetry.properties.dockerPlatform = context.platform;
context.actionContext.telemetry.properties.orchestration = 'single' as DockerOrchestration; // TODO: docker-compose, when support is added
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No longer needed, as the task name is an exact hint as to whether we're dealing with single or compose orchestration.

@@ -166,16 +165,7 @@ export async function selectCommandTemplate(
actionContext.telemetry.properties.commandContextType = `[${selectedTemplate.contextTypes?.join(', ') ?? ''}]`;
actionContext.telemetry.properties.currentContextType = currentContextType;

let resolvedCommand = resolveVariables(selectedTemplate.template, folder, additionalVariables);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since the command template is now based on the setting, this is no longer needed to rewrite commands to match what they used in settings.

karolz-ms
karolz-ms previously approved these changes Mar 30, 2022
Copy link
Contributor

@karolz-ms karolz-ms left a comment

Choose a reason for hiding this comment

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

Very nice fix/refactoring. Just one minor suggestion to consider.

@bwateratmsft bwateratmsft marked this pull request as ready for review March 31, 2022 15:29
@bwateratmsft bwateratmsft requested a review from a team as a code owner March 31, 2022 15:29
@bwateratmsft
Copy link
Collaborator Author

Doc PR: microsoft/vscode-docs#5267

@microsoft microsoft locked and limited conversation to collaborators May 16, 2022
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.

Add a flag to use new Docker Compose command
2 participants