Skip to content

Automatically remove the --platform flag from the Dockerfile on deployment#601

Merged
MicroFish91 merged 9 commits intomainfrom
mwf/platform
Feb 14, 2024
Merged

Automatically remove the --platform flag from the Dockerfile on deployment#601
MicroFish91 merged 9 commits intomainfrom
mwf/platform

Conversation

@MicroFish91
Copy link
Copy Markdown
Contributor

@MicroFish91 MicroFish91 commented Feb 9, 2024

Closes #598

ACR does not support the --platform flag and will automatically error out if it detects this flag. They have an open issue for it, but we have no idea when it will be addressed and enough people are running into it that we have been asked to help address this on our end.

Updated activity log:
image

Big thanks and credit out to Anthony Chu for helping us come up with the bulk of the core logic for custom Dockerfile replacement! :)

@MicroFish91 MicroFish91 marked this pull request as ready for review February 12, 2024 21:26
@MicroFish91 MicroFish91 requested a review from a team as a code owner February 12, 2024 21:26
@alexweininger
Copy link
Copy Markdown
Member

They have an open issue for it, but we have no idea when it will be addressed and enough people are running into it that we should probably try to help address this on our end.

Can you link said issue?

@MicroFish91
Copy link
Copy Markdown
Contributor Author

MicroFish91 commented Feb 13, 2024

Here is the open issue on ACR

Azure/acr#697

// 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 {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the tarFile deleting comes in the runStep immediately after.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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.

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);
Copy link
Copy Markdown
Member

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?

Copy link
Copy Markdown
Contributor Author

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

@MicroFish91 MicroFish91 merged commit 16fb106 into main Feb 14, 2024
@MicroFish91 MicroFish91 deleted the mwf/platform branch February 14, 2024 20:52
@microsoft microsoft locked and limited conversation to collaborators Mar 31, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove build platform flag from Dockerfile prior to deploying

3 participants