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 new Docker SDK-based client #2134

Merged
merged 28 commits into from
Jul 13, 2020
Merged

Add new Docker SDK-based client #2134

merged 28 commits into from
Jul 13, 2020

Conversation

bwateratmsft
Copy link
Collaborator

@bwateratmsft bwateratmsft commented Jul 7, 2020

Fixes #2102

Add the new Docker SDK-based client under the "DockerServe" client implementation. Supported context-menu commands on containers include:

  • View Logs
  • Attach Shell
  • Inspect
  • Open in Browser
  • Remove

These are not* implemented in the Docker SDK yet and will show an error "This action is not supported in the current Docker context.":

  • Restart
  • Start
  • Stop

*Stop is but the behavior is not exactly the same as docker stop, due to the limitations of ACI. We'll leave it unimplemented for now.

@bwateratmsft bwateratmsft requested a review from karolz-ms July 7, 2020 17:07
@bwateratmsft bwateratmsft requested a review from a team as a code owner July 7, 2020 17:07
@@ -81,6 +115,127 @@
}
}
},
"@octokit/auth-token": {
Copy link
Contributor

Choose a reason for hiding this comment

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

This octokit stuff is probably unnecessary. I have opened an issue docker/node-sdk#22

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Webpack's tree shaking is removing the Octokit stuff (as expected), so I think it's OK to merge these changes. We'll pick up a new version of the Docker SDK as soon as one is available.

src/docker/DockerServeClient/DockerServeUtils.ts Outdated Show resolved Hide resolved
@gtardif
Copy link

gtardif commented Jul 8, 2020

landed here following issues from https://github.com/docker/node-sdk ; just noticed "remove" is not planned to be implemented as "not supported in sdk".
You should be able to implement the functionality with the Delete Method.
With the warning that there is a slight semantic difference between ACI and local containers (at least in the command line) : docker rm xxx requires the container to be stopped for local containers. With ACI, docker rm xxx will kill the container and remove it (equivalent of docker rm -f xxx).

@bwateratmsft
Copy link
Collaborator Author

bwateratmsft commented Jul 8, 2020

landed here following issues from https://github.com/docker/node-sdk ; just noticed "remove" is not planned to be implemented as "not supported in sdk".
You should be able to implement the functionality with the Delete Method.
With the warning that there is a slight semantic difference between ACI and local containers (at least in the command line) : docker rm xxx requires the container to be stopped for local containers. With ACI, docker rm xxx will kill the container and remove it (equivalent of docker rm -f xxx).

That was a mis-type by me, Remove will be present. Just stop/start/restart are being turned off (for now). I fixed the initial comment.

let osType = await getDockerOSType(context);
let osType: DockerOSType;
try {
osType = await getDockerOSType(context);
Copy link
Collaborator Author

@bwateratmsft bwateratmsft Jul 9, 2020

Choose a reason for hiding this comment

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

Get OS type from container instead of system? That would allow for Windows containers in ACI to work.

*Need the SDK to provide platform

src/tasks/TaskHelper.ts Outdated Show resolved Hide resolved
@bwateratmsft bwateratmsft merged commit da74716 into master Jul 13, 2020
@bwateratmsft bwateratmsft deleted the bmw/dockerserve branch July 13, 2020 13:47
Dmarch28 pushed a commit to Dmarch28/vscode-docker that referenced this pull request Mar 4, 2021
* Implement client for Docker SDK

* Add another stopped container state

* Assume linux if we can't tell what OS

* Disable stop

* Monkey patch in a label for compose proj

* Add getCurrentContext method
@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.

Support ACI contexts in Explorer Panel
3 participants