Add backward compatibility to API in support of the new managed identity features #816
Conversation
…de-azurecontainerapps into mwf/homely-emerald
…ode-azurecontainerapps into mwf/fiscal-beige
…ode-azurecontainerapps into mwf/fiscal-beige
| @@ -38,12 +48,14 @@ export async function deployWorkspaceProjectApi(deployWorkspaceProjectOptions: a | |||
| rootFolder, | |||
| srcPath: srcPath ? Uri.file(srcPath).fsPath : undefined, | |||
| dockerfilePath: dockerfilePath ? Uri.file(dockerfilePath).fsPath : undefined, | |||
| newRegistryCredentialType: RegistryCredentialType.DockerLogin, | |||
There was a problem hiding this comment.
We need additional discussions about whether we want to default to managed identity here in the future. For now, I'm keeping it as Docker Login to preserve existing behavior.
| @@ -9,7 +9,7 @@ import type * as api from "./vscode-azurecontainerapps.api"; | |||
|
|
|||
| export function getAzureContainerAppsApiProvider(): apiUtils.AzureExtensionApiProvider { | |||
| return createApiProvider([<api.AzureContainerAppsExtensionApi>{ | |||
| // Todo: Change this to 0.0.2 later. 0.0.2 is backwards compatible anyway so this change should be fine either way. | |||
| // Todo: Change this to 0.0.3 later. 0.0.3 is backwards compatible anyway so this change should be fine either way. | |||
There was a problem hiding this comment.
When we eventually add managed identity support for the api, I will bump the major version
There was a problem hiding this comment.
Okay, but just so you know, this changing is purely aesthetic. semver doesn't actually register this as a different version number since there's no major version number.
| shouldSaveDeploySettings: !!shouldSaveDeploySettings, | ||
| }); | ||
|
|
||
| const deployWorkspaceProjectContext: DeployWorkspaceProjectContext = await deployWorkspaceProjectInternal(deployWorkspaceProjectInternalContext, { | ||
| suppressActivity: true, | ||
| suppressConfirmation, | ||
| suppressRegistryPrompt: suppressRegistryPrompt ?? true, |
There was a problem hiding this comment.
This PR introduced the prompt, so add ability to suppress and default it to true to preserve existing behavior
| @@ -17,4 +17,6 @@ export interface DockerLoginRegistryCredentialsContext extends CreateAcrContext, | |||
|
|
|||
| newRegistrySecret?: Secret; | |||
| newRegistryCredential?: RegistryCredentials; | |||
|
|
|||
| suppressEnableAdminUserPrompt?: boolean; | |||
There was a problem hiding this comment.
Can't this just check suppressConfirmation?
There was a problem hiding this comment.
I don't think that prop exists on the context currently. There is the suppressNotification option in user settings for resources, but I don't think we want to follow that here because this prompt is ordinarily one that I would always want to show.
The version of suppressConfirmation we are using here is an explicit option that is being passed into the deployWorkspaceProjectInternal command, primarily added to enable api calls from outside extensions.
Partially addresses #719
Updates the api consumed by functions so that it keeps working in face of all the new features we've added related to managed identities