Fix: Replace TARGET_GOOS and TARGET_GOARCH with TARGET_OS and TARGET_ARCH

[NicolasDorier]

There is a mismatch between the environment name in the docker file and what the makefile is expecting:

• https://github.com/cloudflare/cloudflared/blob/master/Makefile#L78
• https://github.com/cloudflare/cloudflared/blob/master/Makefile#L69

You also need to declare the ARG inside the FROM if you intend to use it in the build stage. See https://docs.docker.com/engine/reference/builder/#understand-how-arg-and-from-interact

[SayakMukhopadhyay]

I think this PR might tie into #618 which reworks the creation of the containers to make it multi platform. That PR and the related issue has been sitting for a while now though.

[sudarshan-reddy]

@NicolasDorier ~I'm going to close this PR because of the reason I stated above. However, do not let this discourage you from opening more PRs. This codebase can be so much better and we will take all the amazing help we can get!~ ❤️

Thanks @SayakMukhopadhyay for pointing out my oversight.

This PR still needs to address https://github.com/cloudflare/cloudflared/pull/681#pullrequestreview-1041104901 and then we can merge it.

[SayakMukhopadhyay]

@sudarshan-reddy just so we are all in the same page, are you sure that TARGET_GOOS and TARGET_GOARCH are getting used in the Makefile? Because I couldn't find an "explicit" mention of these 2 env vars in the Makefile. I could see that GOOS and GOARCH are getting used to set the TARGET_OS and TARGET_ARCH variables in the script but GOOS and GOARCH are not explicitly set in the Dockerfile.

I believe this PR was trying to fix something like that.

[sudarshan-reddy]

Hmm thanks so much for leaving this comment. I've made a mistake. I thought this PR was adding a non-existent variable when in fact it was removing it.

[initdc]

We may met the same problem. https://github.com/AdguardTeam/dnsproxy/pull/228/files#diff-ea8ccbc8293bff3932d8cc333b23b76ce61269aef2b38e253e5335854f340b40R14

https://github.com/cloudflare/cloudflared/pull/618/files#diff-dd2c0eb6ea5cfc6c4bd4eac30934e2d5746747af48fef6da689e85b752f39557R25

We need make one thing clear: docker buildx does not have the ability to create cross-platform binaries because it only takes Linux as an argument.

https://github.com/initdc/ssh-proxy-by-caddy-l4/blob/build/sh/README.md?plain=1#L50

We need to build an array of target platforms to loop runing go build.

https://github.com/initdc/distro-arch-rootfs/blob/master/gen-chart.rb#L10

[SayakMukhopadhyay]

We need make one thing clear: docker buildx does not have the ability to create cross-platform binaries because it only takes Linux as an argument.

https://github.com/initdc/ssh-proxy-by-caddy-l4/blob/build/sh/README.md?plain=1#L50

We need to build an array of target platforms to loop runing go build.

https://github.com/initdc/distro-arch-rootfs/blob/master/gen-chart.rb#L10

That's correct! That's what your PR #618 is doing. I believe this PR is loosely relates to that and does nothing to fix the problem of multi build, only fixes the env vars as a temporary fix, at least that's what I think it is.

[NicolasDorier]

Can we remove the args defined in line 2 and 3?

I think I tried but it didn't work.

Just to let you know: We are building our own cross platform cloudflared image on here. The docker image is btcpayserver/cloudflared:2022.6.3, and we already use it on several architectures based on this fix. We build all arch on a amd64 host. I sadly don't have the time to test variations of this PR, so feel free to close this PR and supersede it with another one.

I believe it would be better to fix that in a deeper way: Rather than using TARGET_GOOS, just using the official GOOS.

[sudarshan-reddy]

Im closing this as we now have docker images that this PR tried to achieve.