Docker context support#1790
Docker context support#1790karolz-ms merged 11 commits intomicrosoft:masterfrom karolz-ms:dev/karolz/docker-context
Conversation
|
Tested on MacOS, Windows and Ubuntu. Still fighting with WSL2+Docker Desktop Edge... |
|
WSL2 is working Will fix tests tomorrow |
| const DOCKER_CONTEXT: string = 'DOCKER_CONTEXT'; | ||
| const DefaultRefreshIntervalSec: number = 20; | ||
| const osp = new LocalOSProvider(); | ||
| const DockerConfigFilePath: string = osp.pathJoin(osp.os, osp.homedir, '.docker', 'config.json'); |
There was a problem hiding this comment.
@philliphoff might disagree so I definitely want him to chime in if he feels strongly--
I'd rather leave LocalOSProvider where it is, and just us os for this. We run our tests across all three platforms which mitigates the need for a unit-test-overridable OS provider. Relatedly, I did some perf analysis recently and the time spent in registerDebugConfigurationProvider (which initializes a lot of these XProviders / XManagers) during extension activation was surprisingly long.
There was a problem hiding this comment.
Sounds good. The rationale for this change is that I needed to construct a file path in a platform-neutral way and the LocalOSProvider functionality was a very good fit.
Also, LocalOSProvider really isn't specific to debugging in any way, and in this case I am just using it to initialize the manager, so no registration is taking place.
There was a problem hiding this comment.
Surprisingly it wasn't the registering, but all the constructing, that was slowing things down. IIRC that single function was around 30-50% of our non-loading-of-files activation time.
There was a problem hiding this comment.
Something fishy must be going on here. LocalOSProvider doesn't even have a constructor defined. And it has no state. It is just a bunch of methods.
There was a problem hiding this comment.
It wasn't only LocalOSProvider, there were a couple dozen parts being constructed and assembled.
There was a problem hiding this comment.
So @philliphoff what do you think about lifting LocalOSProvider into utilities?
There was a problem hiding this comment.
I think it's fine to lift LocalOSProvider out of the debugging folder and make it a more generalized service. I'm not sure I'd switch to os directly (yet) until I saw the data that new'ing even a couple dozen objects was, itself, a significant performance hit (and not a consequence of something else being done in registerDebugConfigurationProvider()). Even then, just dispensing with that individual service wouldn't (one would think) save any significant amount of time, so it would still have benefit from being consistent with the DI pattern of the other components. (As opposed to a larger refactor to optimize load/init performance where we might rethink the DI pattern altogether.)
There was a problem hiding this comment.
@philliphoff @karolz-ms Sounds good. In any case, by far the majority of the activation time is in loading/compiling the JS--by a couple orders of magnitude. The only ways we can improve on that are to reduce our dependencies and to lazy-load, if that's possible.
bwateratmsft
left a comment
bwateratmsft
left a comment
There was a problem hiding this comment.
Looks good to me. There's the one item I added about package.nls.json; and let's reach consensus on the LocalOSProvider thing.
Docker context changes are detected and UI/commands are refreshed accordingly Also cleans up warning suppressions for async Dockerode calls
Co-Authored-By: Brandon Waterloo [MSFT] <36966225+bwateratmsft@users.noreply.github.com>
Co-Authored-By: Brandon Waterloo [MSFT] <36966225+bwateratmsft@users.noreply.github.com>
Docker context changes are detected and UI/commands are refreshed accordingly Also cleans up warning suppressions for async Dockerode calls
3 participants
Docker context changes are detected and UI/commands are refreshed accordingly
Also cleans up warning suppressions for async Dockerode calls and does a few minor refactorings