Generalize registry support #1097

ejizba · 2019-06-28T18:06:36Z

I'm skeptical that we'll ever have time to support all of the providers listed in #869, so my goal with this PR was to make it as easy as possible for the community to add support for their provider. I used GitLab as a test subject, and I'll send out a separate PR so that you can see what that looks like.

The main things that are now generalized:

A log-in wizard that allows for url/username/password
Caching url/username/password across sessions
Pull repos/images
Set registry as default

Since the list of providers will theoretically grow and make the registries view cluttered, the default view will now be this:

Once you select to connect one, you will see this:

That means you will only see the providers you have connected. This is a breaking change from the previous experience and it means all users will essentially have to re-connect to their registry provider when we ship this update. Here's an example of a few registries that are connected:

(You may notice one side-benefit of this work is that you can log in to multiple docker hub accounts 🙂)

Fixes #1091

So that it's much easier for the community to add support for more providers

src/tree/registries/registryPasswords.ts

src/tree/registries/logInWizard/RegistryPasswordStep.ts

src/tree/registries/logInWizard/ILogInWizardContext.ts

src/tree/registries/dockerV2/DockerV2TagTreeItem.ts

src/tree/registries/dockerV2/DockerV2RegistryTreeItemBase.ts

src/tree/registries/azure/AzureAccountTreeItem.ts

src/tree/registries/RegistriesTreeItem.ts

src/tree/registries/IRegistryProvider.ts

PrashanthCorp · 2019-06-28T22:55:39Z

src/commands/registries/pullImages.ts

    }

-    await pullImages(context, node.parent.parent, node.fullTag);
+    await pullImages(context, node.parent.parent, node.repoNameAndTag);
 }

 async function pullImages(context: IActionContext, node: RegistryTreeItemBase, imageRequest: string): Promise<void> {


Not related to this PR. This function name is not apt given that it also pulls repositories.

Also the file name is confusing.

Y'all there's only so much refactoring/file-renaming a person can do at a time 😂

With the caveat that I didn't name this, I think it's named like this because pulling a repository is technically just pulling multiple images at once.

src/commands/registries/dockerHub/openDockerHubInBrowser.ts

PrashanthCorp

Moving from request change to comment since I'll be OOF.

dismissing since he's oof

nturinski · 2019-07-02T22:53:11Z

package.json

                {
                    "command": "vscode-docker.registries.pullImage",
-                    "when": "view == dockerRegistries && viewItem =~ /tag$/i",
+                    "when": "view == dockerRegistries && viewItem =~ /Tag;/",


Pardon my Dockgerance (Docker + ignorance), but why are all the viewItems for the images called Tags?

It's confusing naming for sure and there's no official consensus. Take this as an example:

Generally, latest and v3 are considered tags, repo1 is considered a repository and repo1:latest and repo1:v3 are considered images.

nturinski · 2019-07-02T23:04:32Z

src/tree/registries/registryContextValues.ts

@@ -0,0 +1,63 @@
+/*---------------------------------------------------------------------------------------------


What was the design decision to create this file to parse all of the contextValues instead of letting the treeItemBase classes contain the values? I was wondering since it's a pretty different design pattern from our other repos.

I wanted to create a common pattern for context values so that people wouldn't have to edit package.json when adding new registry providers. The GitLab PR shows that. I found that the previous pattern wasn't flexible enough to support that.

nturinski · 2019-07-02T23:07:08Z

src/commands/registries/azure/deployImageToAzure.ts

@@ -29,7 +34,8 @@ export async function deployImageToAzure(context: IActionContext, node?: RemoteT
        newSiteKind: AppKind.app
    };
    const promptSteps: AzureWizardPromptStep<IAppServiceWizardContext>[] = [];
-    const azureAccountTreeItem = await validateAzureAccountInstalled();
+    // Create a temporary azure account tree item since Azure might not be connected


Connected as in the user isn't logged in yet? Or is there special work to have Azure integrated with Docker (similar to GitHub?)

Connected is referencing the "registries" view. You can deploy a docker hub image to azure, which means you may have never connected the Azure "registries" provider since your images are in Docker Hub, not Azure.

nturinski · 2019-07-02T23:14:13Z

src/commands/registries/azure/deployImageToAzure.ts

-        }
-    } else {
-        imagePath = registryTI.host;
+        password = await registryTI.parent.getPassword();


There used to be an error thrown here if either username or password were undefined. Does getPassword throw an error if it fails to retrieve the password from the keytar or whatever it's using?

99% of the time there will be a username and password. It's not clear what should happen if there isn't, but I decided to err on the side of letting them deploy. There are plenty of self-hosted registries out there with weird auth schemes and I don't want to block people.

nturinski · 2019-07-02T23:15:39Z

src/commands/registries/azure/deployImageToAzure.ts

-    appSettings.push({ name: "DOCKER_REGISTRY_SERVER_USERNAME", value: username });
-    appSettings.push({ name: "DOCKER_REGISTRY_SERVER_PASSWORD", value: password });
+    if (username && password) {
+        appSettings.push({ name: "DOCKER_REGISTRY_SERVER_USERNAME", value: username });


Are these settings only needed for specific types of Registries?

Only ones that need authentication. For example, there are public docker hub images. Plus you could always add these settings later yourself

Or more likely - self hosted registries might exist that don't have auth setup yet

nturinski · 2019-07-02T23:19:44Z

src/commands/registries/azure/untagAzureImage.ts

    }

-    const confirmUntag: string = `Are you sure you want to untag "${node.fullTag}"? This does not delete the manifest referenced by the tag.`;
+    const confirmUntag: string = `Are you sure you want to untag "${node.repoNameAndTag}"? This does not delete the manifest referenced by the tag.`;


Since this isn't destructive, do we need to prompt for it? I realize that you aren't the one who made it a prompt, just wondering if we needed to have it.

It is (partially) destructive

nturinski · 2019-07-02T23:23:32Z

src/commands/registries/dockerHub/openDockerHubInBrowser.ts

@@ -22,7 +23,8 @@ export async function openDockerHubInBrowser(context: IActionContext, node?: Doc
    } else if (node instanceof DockerHubRepositoryTreeItem) {
        url += `r/${node.parent.namespace}/${node.repoName}`;
    } else {
-        url += `r/${node.parent.parent.namespace}/${node.parent.repoName}/tags`
+        const repoTI = <DockerHubRepositoryTreeItem>node.parent;
+        url += `r/${repoTI.parent.namespace}/${repoTI.repoName}/tags`;


Do you know why it's using += here? It doesn't look like there was anything assigned to dockerHubUrl previously anyway

dockerHubUrl is defined in constants.ts

nturinski · 2019-07-02T23:27:44Z

src/commands/registries/logOutOfDockerCli.ts

    const terminal: Terminal = ext.terminalProvider.createTerminal('docker logout');
-    terminal.sendText(command);
+    terminal.sendText(`docker logout ${creds.registryPath}`);


Does it make sense to replace all of the terminal calls with dockerode?

I've considered it, but that's a pretty big refactor and I've probably already gone too far down the refactoring rabbit-hole lately

nturinski · 2019-07-02T23:31:33Z

src/tree/registerTrees.ts

-    ext.registriesTree = new AzExtTreeDataProvider(registriesTreeItem, registriesLoadMore);
-    ext.registriesTreeView = window.createTreeView('dockerRegistries', { treeDataProvider: ext.registriesTree });
+    ext.registriesTree = new AzExtTreeDataProvider(ext.registriesRoot, registriesLoadMore);
+    ext.registriesTreeView = window.createTreeView('dockerRegistries', { treeDataProvider: ext.registriesTree, showCollapseAll: true });


🎉 🎉 🎉 🎉 🎉

nturinski · 2019-07-03T16:49:04Z

src/tree/registries/RegistriesTreeItem.ts

-                    break;
-                }
+        let placeHolder: string = 'Select the provider for your registry';
+        const provider = (await ext.ui.showQuickPick(picks, { placeHolder, suppressPersistence: true })).data;


Don't we have a learnMore property that we can leverage?

That's only for warning messages, not quick picks. Also that logic will re-show the message, but there's no point of re-showing the quick pick if your provider isn't listed

Generalize registry support

23c73ca

So that it's much easier for the community to add support for more providers

ejizba requested a review from a team as a code owner June 28, 2019 18:06

This was referenced Jun 28, 2019

Add GitLab support #1098

Merged

docker error vs code #996

Closed

Better handle situation where azure account is not installed microsoft/vscode-azuretools#525

Merged

PrashanthCorp previously requested changes Jun 28, 2019

View reviewed changes

ejizba added 3 commits June 28, 2019 17:17

PR Feedback

ed97973

Remove ACR per Mr Matt

4ca0086

Bump ui package version

9206a37

PrashanthCorp reviewed Jul 1, 2019

View reviewed changes

ejizba added 2 commits July 2, 2019 08:04

Log In -> Sign in

efb7c5f

minor fixes

a789c7e

nturinski reviewed Jul 2, 2019

View reviewed changes

nturinski reviewed Jul 3, 2019

View reviewed changes

nturinski approved these changes Jul 3, 2019

View reviewed changes

ejizba merged commit 7db7592 into master Jul 3, 2019

ejizba deleted the ej/regV3 branch July 4, 2019 00:10

microsoft locked and limited conversation to collaborators Oct 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generalize registry support #1097

Generalize registry support #1097

ejizba commented Jun 28, 2019 •

edited

Loading

PrashanthCorp Jun 28, 2019

nturinski Jul 2, 2019

ejizba Jul 2, 2019

PrashanthCorp left a comment •

edited

Loading

nturinski Jul 2, 2019

ejizba Jul 2, 2019

nturinski Jul 2, 2019

ejizba Jul 2, 2019

nturinski Jul 2, 2019

ejizba Jul 2, 2019

nturinski Jul 2, 2019

ejizba Jul 2, 2019

nturinski Jul 2, 2019

ejizba Jul 2, 2019

ejizba Jul 2, 2019

nturinski Jul 2, 2019

ejizba Jul 2, 2019

nturinski Jul 2, 2019

ejizba Jul 2, 2019

nturinski Jul 2, 2019

ejizba Jul 2, 2019

nturinski Jul 2, 2019

nturinski Jul 3, 2019

ejizba Jul 3, 2019

		@@ -0,0 +1,63 @@
		/*---------------------------------------------------------------------------------------------

Generalize registry support #1097

Generalize registry support #1097

Conversation

ejizba commented Jun 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PrashanthCorp left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ejizba commented Jun 28, 2019 •

edited

Loading

PrashanthCorp left a comment •

edited

Loading