-
Notifications
You must be signed in to change notification settings - Fork 17
Automatically remove the --platform flag from the Dockerfile on deployment
#601
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
9c540ff
076cf21
5c08a88
d3e17f7
fa60147
6c552df
3672906
900ad4e
147c477
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,9 +5,12 @@ | |
|
|
||
| import { getResourceGroupFromId } from '@microsoft/vscode-azext-azureutils'; | ||
| import { AzExtFsExtra, GenericParentTreeItem, GenericTreeItem, activityFailContext, activityFailIcon, activitySuccessContext, activitySuccessIcon, nonNullValue } from '@microsoft/vscode-azext-utils'; | ||
| import { randomUUID } from 'crypto'; | ||
| import { tmpdir } from 'os'; | ||
| import * as path from 'path'; | ||
| import * as tar from 'tar'; | ||
| import { type Progress } from 'vscode'; | ||
| import { ThemeColor, ThemeIcon, type Progress } from 'vscode'; | ||
| import { ext } from '../../../../extensionVariables'; | ||
| import { ExecuteActivityOutputStepBase, type ExecuteActivityOutput } from '../../../../utils/activity/ExecuteActivityOutputStepBase'; | ||
| import { createActivityChildContext } from '../../../../utils/activity/activityUtils'; | ||
| import { createContainerRegistryManagementClient } from '../../../../utils/azureClients'; | ||
|
|
@@ -16,12 +19,14 @@ import { type BuildImageInAzureImageSourceContext } from './BuildImageInAzureIma | |
|
|
||
| const vcsIgnoreList = ['.git', '.gitignore', '.bzr', 'bzrignore', '.hg', '.hgignore', '.svn']; | ||
|
|
||
| export class UploadSourceCodeStep extends ExecuteActivityOutputStepBase<BuildImageInAzureImageSourceContext> { | ||
| export class UploadSourceCodeStep<T extends BuildImageInAzureImageSourceContext> extends ExecuteActivityOutputStepBase<T> { | ||
| public priority: number = 430; | ||
| /** Path to a directory containing a custom Dockerfile that we sometimes build and upload for the user */ | ||
| private _customDockerfileDirPath?: string; | ||
| /** Relative path of src folder from rootFolder and what gets deployed */ | ||
| private _sourceFilePath: string; | ||
|
|
||
| protected async executeCore(context: BuildImageInAzureImageSourceContext, progress: Progress<{ message?: string | undefined; increment?: number | undefined }>): Promise<void> { | ||
| protected async executeCore(context: T, progress: Progress<{ message?: string | undefined; increment?: number | undefined }>): Promise<void> { | ||
| this._sourceFilePath = context.rootFolder.uri.fsPath === context.srcPath ? '.' : path.relative(context.rootFolder.uri.fsPath, context.srcPath); | ||
| context.telemetry.properties.sourceDepth = this._sourceFilePath === '.' ? '0' : String(this._sourceFilePath.split(path.sep).length); | ||
|
|
||
|
|
@@ -35,7 +40,31 @@ export class UploadSourceCodeStep extends ExecuteActivityOutputStepBase<BuildIma | |
| let items = await AzExtFsExtra.readDirectory(source); | ||
| items = items.filter(i => !vcsIgnoreList.includes(i.name)); | ||
|
|
||
| await tar.c({ cwd: source, gzip: true, file: context.tarFilePath }, items.map(i => path.relative(source, i.fsPath))); | ||
| await this.buildCustomDockerfileIfNecessary(context); | ||
| if (this._customDockerfileDirPath) { | ||
| // Create an uncompressed tarball with the base project | ||
| const tempTarFilePath: string = context.tarFilePath.replace(/\.tar\.gz/, '.tar'); | ||
| await tar.c({ cwd: source, file: tempTarFilePath }, items.map(i => path.relative(source, i.fsPath))); | ||
|
|
||
| // Append/Overwrite the original Dockerfile with the custom one that was made | ||
| await tar.r({ cwd: this._customDockerfileDirPath, file: tempTarFilePath }, [path.relative(source, context.dockerfilePath)]); | ||
|
|
||
| // Create the final compressed version | ||
| // Note: Noticed some hanging issues when using the async version to add existing tar archives; | ||
| // however, the issues seem to disappear when utilizing the sync version | ||
| tar.c({ cwd: tmpdir(), gzip: true, sync: true, file: context.tarFilePath }, [`@${path.basename(tempTarFilePath)}`]); | ||
|
|
||
| try { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this try/catch supposed to only be in the if clause? I thought we need to delete the tarFile that we create regardless of the custom logic.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe the tarFile deleting comes in the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In general I have some thoughts about/want some feedback regarding the try/catch structure though. I'll reach out to discuss more offline. |
||
| // Remove temporarily created resources | ||
| await AzExtFsExtra.deleteResource(tempTarFilePath); | ||
| await AzExtFsExtra.deleteResource(this._customDockerfileDirPath, { recursive: true }); | ||
| } catch { | ||
| // Swallow error, don't halt the deploy process just because we couldn't delete the temp files, provide a warning instead | ||
| ext.outputChannel.appendLog(localize('errorDeletingTempFiles', 'Warning: Could not remove some of the following temporary files: "{0}", "{1}". Try removing these manually at a later time.', tempTarFilePath, this._customDockerfileDirPath)); | ||
| } | ||
| } else { | ||
| await tar.c({ cwd: source, gzip: true, file: context.tarFilePath }, items.map(i => path.relative(source, i.fsPath))); | ||
| } | ||
|
|
||
| const sourceUploadLocation = await context.client.registries.getBuildSourceUploadUrl(context.resourceGroupName, context.registryName); | ||
| const uploadUrl: string = nonNullValue(sourceUploadLocation.uploadUrl); | ||
|
|
@@ -48,22 +77,64 @@ export class UploadSourceCodeStep extends ExecuteActivityOutputStepBase<BuildIma | |
| context.uploadedSourceLocation = relativePath; | ||
| } | ||
|
|
||
| public shouldExecute(context: BuildImageInAzureImageSourceContext): boolean { | ||
| public shouldExecute(context: T): boolean { | ||
| return !context.uploadedSourceLocation; | ||
| } | ||
|
|
||
| protected createSuccessOutput(context: BuildImageInAzureImageSourceContext): ExecuteActivityOutput { | ||
| /** | ||
| * Checks and creates a custom Dockerfile if necessary to be used in place of the original | ||
| * @populates this._customDockerfileDirPath | ||
| */ | ||
| private async buildCustomDockerfileIfNecessary(context: T): Promise<void> { | ||
| // Build a custom Dockerfile if it has ACR's unsupported `--platform` flag | ||
| // See: https://github.com/Azure/acr/issues/697 | ||
| const platformRegex: RegExp = /^(FROM.*)\s--platform=\S+(.*)$/gm; | ||
| let dockerfileContent: string = await AzExtFsExtra.readFile(context.dockerfilePath); | ||
|
|
||
| if (!platformRegex.test(dockerfileContent)) { | ||
| return; | ||
| } | ||
|
|
||
| ext.outputChannel.appendLog(localize('removePlatformFlag', 'Detected a "--platform" flag in the Dockerfile. This flag is not supported in ACR. Attempting to provide a Dockerfile with the "--platform" flag removed.')); | ||
| dockerfileContent = dockerfileContent.replace(platformRegex, '$1$2'); | ||
|
|
||
| const customDockerfileDirPath: string = path.join(tmpdir(), randomUUID()); | ||
| const dockerfileRelativePath: string = path.relative(context.srcPath, context.dockerfilePath); | ||
| const customDockerfilePath = path.join(customDockerfileDirPath, dockerfileRelativePath); | ||
| await AzExtFsExtra.writeFile(customDockerfilePath, dockerfileContent); | ||
|
|
||
| this._customDockerfileDirPath = customDockerfileDirPath; | ||
| } | ||
|
|
||
| protected createSuccessOutput(context: T): ExecuteActivityOutput { | ||
| const baseTreeItemOptions = { | ||
| contextValue: createActivityChildContext(['uploadSourceCodeStepSuccessItem', activitySuccessContext]), | ||
| label: localize('uploadSourceCodeLabel', 'Upload source code from "{1}" directory to registry "{0}"', context.registry?.name, this._sourceFilePath), | ||
| iconPath: activitySuccessIcon, | ||
| }; | ||
|
|
||
| let parentTreeItem: GenericParentTreeItem | undefined; | ||
| if (this._customDockerfileDirPath) { | ||
| parentTreeItem = new GenericParentTreeItem(undefined, { | ||
| ...baseTreeItemOptions, | ||
| loadMoreChildrenImpl: () => { | ||
| const removePlatformFlagItem = new GenericTreeItem(undefined, { | ||
| contextValue: createActivityChildContext(['removePlatformFlagItem']), | ||
| label: localize('removePlatformFlag', 'Remove unsupported ACR "--platform" flag'), | ||
| iconPath: new ThemeIcon('dash', new ThemeColor('terminal.ansiWhite')), | ||
| }); | ||
| return Promise.resolve([removePlatformFlagItem]); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| return { | ||
| item: new GenericTreeItem(undefined, { | ||
| contextValue: createActivityChildContext(['uploadSourceCodeStepSuccessItem', activitySuccessContext]), | ||
| label: localize('uploadSourceCodeLabel', 'Upload source code from "{1}" directory to registry "{0}"', context.registry?.name, this._sourceFilePath), | ||
| iconPath: activitySuccessIcon | ||
| }), | ||
| item: parentTreeItem ?? new GenericTreeItem(undefined, { ...baseTreeItemOptions }), | ||
| message: localize('uploadedSourceCodeSuccess', 'Uploaded source code from "{1}" directory to registry "{0}" for remote build.', context.registry?.name, this._sourceFilePath) | ||
| }; | ||
| } | ||
|
|
||
| protected createFailOutput(context: BuildImageInAzureImageSourceContext): ExecuteActivityOutput { | ||
| protected createFailOutput(context: T): ExecuteActivityOutput { | ||
| return { | ||
| item: new GenericParentTreeItem(undefined, { | ||
| contextValue: createActivityChildContext(['uploadSourceCodeStepFailItem', activityFailContext]), | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this should be in a the try/catch too? I don't know; if this fails, maybe it's not worth trying the build with their dockerfile anyway?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a few points here that an error could be thrown that I haven't wrapped with try/catch to preserve readability and because I don't necessarily have an elegant solution for error handling in mind. Let's discuss this offline as well