deployImageApi implementation
#229
Conversation
MicroFish91
changed the title
deployImageApi command implementationdeployImageApi implementation
We can break up the image name in the Docker extension to pretty much any degree required, so if a slightly different format for the passed-in data is easier, feel free to change it. |
MicroFish91
force-pushed
the
mwf/deployImageApi
branch
from
de35888 to
4b3e056
Compare
MicroFish91
force-pushed
the
mwf/deployImageApi
branch
from
c64e64e to
1326054
Compare
src/utils/imageNameUtils.ts
Outdated
| import { localize } from "./localize"; | ||
|
|
||
| export namespace imageNameUtils { | ||
| export function detectRegistryDomain(imageName: string): SupportedRegistries | undefined { |
There was a problem hiding this comment.
I'm a little leery of the regex approach here, since there's potential for mistakes. For example, if I supply an image with the name "azurecr.io.foo/bar:baz", it would be incorrectly detected as belonging to ACR. True, this example is contrived, but my gut is telling me something else could happen.
We can supply the registry domain specifically, as part of the contract. The Docker extension already has some fairly sophisticated image name parsers that we trust. Given the domain, a more exact regex could be applied, such as /\.azurecr\.io$/i (which demands "azurecr.io" be the end of the domain, and that it start with a dot, e.g. "myfoo.azurecr.io" passes, but "myfoo.azurecr.iot" does not). For Docker Hub, I think /^docker\.io$/i is sufficient.
There was a problem hiding this comment.
Let me know if you think something like this would be ok!
Instead of amending the contract, just parsing the registryName off of the imageName and doing those regex searches on that value.
There was a problem hiding this comment.
Instead of loginServer, could we just send registry (the domain), and for Docker Hub's case just map loginServer to the other value it wants? IIRC, for ACR and others the login server === registry domain right? Generally speaking I'd prefer to keep all the parsing/splitting in the Docker extension, but having a straightforward mapping of registry -> login server seems safe.
There was a problem hiding this comment.
Sounds good. I have a couple things I want to clarify with you offline before making the changes
MicroFish91
force-pushed
the
mwf/deployImageApi
branch
from
b1cafc8 to
1a77e09
Compare
MicroFish91
force-pushed
the
mwf/deployImageApi
branch
from
1a77e09 to
f65fca3
Compare
MicroFish91
force-pushed
the
mwf/deployImageApi
branch
from
1c1bec3 to
fa14742
Compare
MicroFish91
force-pushed
the
mwf/deployImageApi
branch
from
fa14742 to
3b4dc9f
Compare
|
Ready for review. Latest work contingent on the changes from this PR |
* Add command for deploying to ACA from Docker extension * Explicitly make node undefined * `deployImageApi` implementation (#229) * Fix bad merge * Alex's feedback * Export not needed --------- Co-authored-by: Matthew Fisher <[email protected]>
Edit: Ready for review.