fix: use linux/amd64 platform only for m1/m2 macs (arm64) (#2986)

* fix: add logic for adding platform flag when arch is arm64

* refactor: update buildImage call in container:push command

* test: add tests for buildImage function

* test: update container:push tests

* update comment on logic adding platform flag

packages/cli/src/commands/container/push.ts

@@ -92,7 +92,13 @@ export default class Push extends Command {
           ux.styledHeader(`Building ${job.name} (${job.dockerfile})`)
         }
 
-        await DockerHelper.buildImage(job.dockerfile, job.resource, buildArgs, contextPath)
+        await DockerHelper.buildImage({
+          dockerfile: job.dockerfile,
+          resource: job.resource,
+          buildArgs,
+          path: contextPath,
+          arch: this.config.arch,
+        })
       }
     } catch (error) {
       ux.error(`docker build exited with ${error}`, {exit: 1})

packages/cli/src/lib/container/docker_helper.ts

@@ -169,9 +169,20 @@ export const chooseJobs = async function (jobs: groupedDockerJobs) {
   return chosenJobs
 }
 
-export const buildImage = async function (dockerfile: string, resource: string, buildArgs: string[], path?: string) {
+type BuildImageParams = {
+  dockerfile: string,
+  resource: string,
+  buildArgs: string[],
+  path?: string,
+  arch?: string,
+}
+
+export const buildImage = async function ({dockerfile, resource, buildArgs, path, arch}: BuildImageParams) {
   const cwd = path || Path.dirname(dockerfile)
-  const args = ['build', '-f', dockerfile, '-t', resource, '--platform', 'linux/amd64']
+  const args = ['build', '-f', dockerfile, '-t', resource]
+  // Older Docker versions don't allow for this flag, but we are
+  // adding it here when necessary to allow for pushing a docker build from m1/m2 Macs.
+  if (arch === 'arm64') args.push('--platform', 'linux/amd64')
 
   for (const element of buildArgs) {
     if (element.length > 0) {

packages/cli/test/unit/commands/container/push.unit.test.ts

@@ -56,7 +56,6 @@ describe('container push', function () {
       const dockerfiles = sandbox.stub(DockerHelper, 'getDockerfiles')
         .returns(['/path/to/Dockerfile'])
       const build = sandbox.stub(DockerHelper, 'buildImage')
-        .withArgs('/path/to/Dockerfile', 'registry.heroku.com/testapp/web', [])
       const push = sandbox.stub(DockerHelper, 'pushImage')
         .withArgs('registry.heroku.com/testapp/web')
 
@@ -70,6 +69,8 @@ describe('container push', function () {
       expect(stdout.output).to.contain('Pushing web (/path/to/Dockerfile)')
       sandbox.assert.calledOnce(dockerfiles)
       sandbox.assert.calledOnce(build)
+      expect(build.getCall(0).args[0].dockerfile).to.equal('/path/to/Dockerfile')
+      expect(build.getCall(0).args[0].resource).to.equal('registry.heroku.com/testapp/web')
       sandbox.assert.calledOnce(push)
     })
   })
@@ -133,7 +134,6 @@ describe('container push', function () {
       const dockerfiles = sandbox.stub(DockerHelper, 'getDockerfiles')
         .returns(['/path/to/Dockerfile'])
       const build = sandbox.stub(DockerHelper, 'buildImage')
-        .withArgs('/path/to/Dockerfile', 'registry.heroku.com/testapp/web', [])
       const push = sandbox.stub(DockerHelper, 'pushImage')
         .withArgs('registry.heroku.com/testapp/web')
 
@@ -147,14 +147,15 @@ describe('container push', function () {
       expect(stdout.output).to.contain('Pushing web (/path/to/Dockerfile)')
       sandbox.assert.calledOnce(dockerfiles)
       sandbox.assert.calledOnce(build)
+      expect(build.getCall(0).args[0].dockerfile).to.equal('/path/to/Dockerfile')
+      expect(build.getCall(0).args[0].resource).to.equal('registry.heroku.com/testapp/web')
       sandbox.assert.calledOnce(push)
     })
 
     it('pushes the standard dockerfile to non-web', async function () {
       const dockerfiles = sandbox.stub(DockerHelper, 'getDockerfiles')
         .returns(['/path/to/Dockerfile'])
       const build = sandbox.stub(DockerHelper, 'buildImage')
-        .withArgs('/path/to/Dockerfile', 'registry.heroku.com/testapp/worker', [])
       const push = sandbox.stub(DockerHelper, 'pushImage')
         .withArgs('registry.heroku.com/testapp/worker')
 
@@ -168,15 +169,15 @@ describe('container push', function () {
       expect(stdout.output).to.contain('Pushing worker (/path/to/Dockerfile)')
       sandbox.assert.calledOnce(dockerfiles)
       sandbox.assert.calledOnce(build)
+      expect(build.getCall(0).args[0].dockerfile).to.equal('/path/to/Dockerfile')
+      expect(build.getCall(0).args[0].resource).to.equal('registry.heroku.com/testapp/worker')
       sandbox.assert.calledOnce(push)
     })
 
     it('pushes specified dockerfiles recursively', async function () {
       const dockerfiles = sandbox.stub(DockerHelper, 'getDockerfiles')
         .returns(['/path/to/Dockerfile.web', '/path/to/Dockerfile.worker'])
       const build = sandbox.stub(DockerHelper, 'buildImage')
-      build.withArgs('/path/to/Dockerfile.web', 'registry.heroku.com/testapp/web', [])
-      build.withArgs('/path/to/Dockerfile.worker', 'registry.heroku.com/testapp/worker', [])
       const push = sandbox.stub(DockerHelper, 'pushImage')
       push.withArgs('registry.heroku.com/testapp/web')
       push.withArgs('registry.heroku.com/testapp/worker')
@@ -195,6 +196,8 @@ describe('container push', function () {
       expect(stdout.output).to.contain('Pushing worker (/path/to/Dockerfile.worker)')
       sandbox.assert.calledOnce(dockerfiles)
       sandbox.assert.calledTwice(build)
+      expect(build.getCall(0).args[0].dockerfile).to.equal('/path/to/Dockerfile.web')
+      expect(build.getCall(1).args[0].dockerfile).to.equal('/path/to/Dockerfile.worker')
       sandbox.assert.calledTwice(push)
     })
 
@@ -228,8 +231,6 @@ describe('container push', function () {
       const dockerfiles = sandbox.stub(DockerHelper, 'getDockerfiles')
         .returns(['/path/to/Dockerfile.web', '/path/to/Dockerfile.worker'])
       const build = sandbox.stub(DockerHelper, 'buildImage')
-      build.withArgs('/path/to/Dockerfile.web', 'registry.heroku.com/testapp/web', [])
-      build.withArgs('/path/to/Dockerfile.worker', 'registry.heroku.com/testapp/worker', [])
       const push = sandbox.stub(DockerHelper, 'pushImage')
       push.withArgs('registry.heroku.com/testapp/web')
       push.withArgs('registry.heroku.com/testapp/worker')
@@ -246,14 +247,15 @@ describe('container push', function () {
       expect(stdout.output).to.contain('Pushing worker (/path/to/Dockerfile.worker)')
       sandbox.assert.calledOnce(dockerfiles)
       sandbox.assert.calledTwice(build)
+      expect(build.getCall(0).args[0].dockerfile).to.equal('/path/to/Dockerfile.web')
+      expect(build.getCall(1).args[0].dockerfile).to.equal('/path/to/Dockerfile.worker')
       sandbox.assert.calledTwice(push)
     })
 
     it('builds with custom context path and pushes to the docker registry', async function () {
       const dockerfiles = sandbox.stub(DockerHelper, 'getDockerfiles')
         .returns(['/path/to/Dockerfile'])
       const build = sandbox.stub(DockerHelper, 'buildImage')
-        .withArgs('/path/to/Dockerfile', 'registry.heroku.com/testapp/web', [], '/custom/context/path')
       const push = sandbox.stub(DockerHelper, 'pushImage')
         .withArgs('registry.heroku.com/testapp/web')
 
@@ -265,10 +267,15 @@ describe('container push', function () {
         'web',
       ])
 
+      const buildCallArgs = build.getCall(0).args[0]
+
       expect(stdout.output).to.contain('Building web (/path/to/Dockerfile)')
       expect(stdout.output).to.contain('Pushing web (/path/to/Dockerfile)')
       sandbox.assert.calledOnce(dockerfiles)
       sandbox.assert.calledOnce(build)
+      expect(buildCallArgs.dockerfile).to.equal('/path/to/Dockerfile')
+      expect(buildCallArgs.resource).to.equal('registry.heroku.com/testapp/web')
+      expect(buildCallArgs.path).to.equal('/custom/context/path')
       sandbox.assert.calledOnce(push)
     })
 

packages/cli/test/unit/lib/container/docker_helper.unit.test.ts

@@ -215,4 +215,57 @@ describe('DockerHelper', function () {
       expect(eventStub.calledOnce).to.equal(true)
     })
   })
+
+  describe('.buildImage', function () {
+    const sandbox = sinon.createSandbox()
+
+    afterEach(function () {
+      return sandbox.restore()
+    })
+
+    it('does not include the platform flag when the arch is not arm64', async function () {
+      const argsArray = [
+        'build',
+        '-f',
+        'dockerfile',
+        '-t',
+        'registry.heroku.com/test-app/web',
+        '.',
+      ]
+
+      const eventStub = sandbox.stub(childProcess, 'spawn').callsFake(eventMock)
+
+      await DockerHelper.buildImage({
+        dockerfile: 'dockerfile',
+        resource: 'registry.heroku.com/test-app/web',
+        buildArgs: [],
+      })
+      const options = eventStub.getCall(0).args[2]
+      console.log(eventStub.getCall(0).args)
+      expect(eventStub.calledWith('docker', argsArray, options)).to.equal(true)
+    })
+
+    it('includes the platform flag when arch is arm64 (m1/m2 Macs)', async function () {
+      const argsArray = [
+        'build',
+        '-f',
+        'dockerfile',
+        '-t',
+        'registry.heroku.com/test-app/web',
+        '--platform',
+        'linux/amd64',
+        '.',
+      ]
+      const eventStub = sandbox.stub(childProcess, 'spawn').callsFake(eventMock)
+
+      await DockerHelper.buildImage({
+        dockerfile: 'dockerfile',
+        resource: 'registry.heroku.com/test-app/web',
+        buildArgs: [],
+        arch: 'arm64',
+      })
+      const options = eventStub.getCall(0).args[2]
+      expect(eventStub.calledWith('docker', argsArray, options)).to.equal(true)
+    })
+  })
 })