Support private registries #381
Conversation
|
ping, thanks |
commands/utils/TerminalProvider.ts
Outdated
| this.sendText(command); | ||
| } | ||
|
|
||
| let results = await this.waitForCompletionCore({ ignoreErrors: options.ignoreErrors }); |
There was a problem hiding this comment.
nit: You can just pass in options
| } | ||
|
|
||
| // Remove files in preparation for next commands, if any | ||
| await fse.remove(this._semaphorePath); |
There was a problem hiding this comment.
Do we expect tests to run multiple things from the same terminal? I'm wondering if this should be in the dispose method instead
There was a problem hiding this comment.
Yes, I'm currently running multiple commands sometimes in tests. It's also possible we might add a setting to allow the user to do that eventually. I think this is safer and more flexible.
| @@ -0,0 +1,110 @@ | |||
| import * as ContainerModels from 'azure-arm-containerregistry/lib/models'; | |||
| } | ||
|
|
||
| export async function getTags(registryUrl: string, repositoryName: string, credentials?: RegistryCredentials): Promise<TagInfo[]> { | ||
| let result = await registryRequest<{ tags: string[] }>(registryUrl, `v2/${repositoryName}/tags/list?page_size=100&page=1`, credentials); |
There was a problem hiding this comment.
So does this only get 100? Can we pull that out into a constant?
explorer/models/customRegistries.ts
Outdated
| credentials: { userName, password } | ||
| }; | ||
|
|
||
| let invalidMessage = await CustomRegistryNode.isValidRegistryUrl(newRegistry); |
There was a problem hiding this comment.
So if I understand isValidRegistryUrl correctly, this is what's happening:
let invalidMessage: string | undefined;
try {
// check registry
} catch (error) {
invalidMessage = parseError(error).message;
}
if (invalidMessage) {
throw new Error(invalidMessage);
}Which could just be written as this, correct?:
// check registryThere was a problem hiding this comment.
:-) Artifacts from an original design.
| repoNodes.push(new CustomRepositoryNode(repoName, this.registry)); | ||
| } | ||
| } catch (error) { | ||
| vscode.window.showErrorMessage(parseError(error).message); |
There was a problem hiding this comment.
Do you care that this will never be tracked in telemetry?
There was a problem hiding this comment.
There's already a work item for it: #358
| function getKeytarModule(): typeof keytarType { | ||
| const keytar: typeof keytarType | undefined = getCoreNodeModule('keytar'); | ||
| if (!keytar) { | ||
| throw new Error("Internal error: Could not find keytar module for reading and writing passwords"); |
There was a problem hiding this comment.
Unless you can prove this extension is useless without keytar, I think we should keep it optional (aka don't throw an error here and change the type to | undefined in ext like it was before your changes). We're relying on something shipped with VS Code and I just don't think we can assume it will always be there (I mean they've already changed it once).
| @@ -275,6 +275,8 @@ export class RootNode extends NodeBase { | |||
| registryRootNodes.push(new RegistryRootNode('Azure', "azureRegistryRootNode", this.eventEmitter, this._azureAccount)); | |||
| } | |||
|
|
|||
| registryRootNodes.push(new RegistryRootNode('Private registries', 'customRootNode', null)); | |||
There was a problem hiding this comment.
Feels like this is more of Attached registries. In other words, couldn't I use this to attach all types of things - including private/public/custom/Azure? Also - attached is consistent with Cosmos DB
There was a problem hiding this comment.
Right now it only handles certain types of registries, we felt 'private registries' probably indicates that best. Might change later.
package.json
Outdated
| @@ -185,7 +187,7 @@ | |||
| }, | |||
| { | |||
| "command": "vscode-docker.createWebApp", | |||
| "when": "view == dockerExplorer && viewItem =~ /^(azureImageNode|dockerHubImageTag)$/" | |||
| "when": "view == dockerExplorer && viewItem =~ /^(azureImageTagNode|dockerHubImageTag|customImageTag)$/" | |||
There was a problem hiding this comment.
nit: I'd prefer something like /^(azure|dockerHub|custom)ImageTag/$ (NOTE: You'd have to remove Node off of the Azure one or add it to the other ones to be consistent)
There was a problem hiding this comment.
I'd rather not do it that way because it makes them harder to search for and also assumes something about their structure. Did take the time to make these more consistent, though.
utils/keytar.ts
Outdated
| } | ||
| } | ||
|
|
||
| export class TestKeytar implements IKeytar { |
There was a problem hiding this comment.
nit: I prefer to keep all test code in the test folder
explorer/models/customRegistries.ts
Outdated
|
|
||
| async function saveCustomRegistriesNonsensitive(registries: CustomRegistry[]): Promise<void> { | ||
| let minimal: CustomRegistryNonsensitive[] = registries.map(reg => <CustomRegistryNonsensitive>{ url: reg.url }); | ||
| await ext.context.workspaceState.update(customRegistriesKey, minimal); |
There was a problem hiding this comment.
You missed this one (changing workspaceState to globalState)
There was a problem hiding this comment.
Fixed, thanks.
Fixes #18
Testcases added to OneNote tab "Custom registries"