Skip to content
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

Add build for ARM64, remove assumption of running on AMD64 #5284

Closed
wants to merge 2 commits into from

Conversation

backjo
Copy link
Contributor

@backjo backjo commented Jan 21, 2021

This PR does two things - it adds a new build target for the cli (linux arm64), and it removes an assumption that the 'argocd' binary is built for amd64. This is a bug we noticed when running Argo CD on ARM-based instances - downloading the linux-amd64 binary for CLI (argocd-linux-amd64) was actually built for arm64, since the container was running on ARM64.

Signed-off-by: Jonah Back [email protected]

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
  • The title of the PR states what changed and the related issues number (used for the release note).
  • I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
  • Does this PR require documentation updates?
  • I've updated documentation as required by this PR.
  • Optional. My organization is added to USERS.md.
  • I have signed off all my commits as required by DCO
  • My build is green (troubleshooting builds).

@codecov-io
Copy link

Codecov Report

Merging #5284 (3518a07) into master (7af5837) will increase coverage by 0.00%.
The diff coverage is 50.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #5284   +/-   ##
=======================================
  Coverage   41.07%   41.07%           
=======================================
  Files         136      136           
  Lines       18365    18372    +7     
=======================================
+ Hits         7544     7547    +3     
- Misses       9742     9745    +3     
- Partials     1079     1080    +1     
Impacted Files Coverage Δ
server/server.go 54.37% <50.00%> (-0.16%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7af5837...3518a07. Read the comment docs.

Dockerfile Outdated
make BIN_NAME=argocd-windows-amd64.exe GOOS=windows argocd-all \
make BIN_NAME=argocd-windows-amd64.exe GOOS=windows argocd-all && \
make BIN_NAME=argocd-linux-amd64 GOOS=linux GOARCH=amd64 argocd-all && \
make BIN_NAME=argocd-linux-arm64 GOOS=linux GOARCH=arm64 argocd-all \
Copy link
Contributor

Choose a reason for hiding this comment

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

Correct me if I am wrong. But, this change will create an additional binary (85MB est) which is duplicate of argocd binary built here

@kshamajain99
Copy link
Contributor

@backjo argocd is aiming to keep image size lower (refer #5005). Can we try to optimize this PR in order to not increase binary size significantly.

@backjo
Copy link
Contributor Author

backjo commented Jan 25, 2021

Latest commit should help keep the image size down - instead of adding an additional binary, if BUILD_ALL_CLIS is set, it will use a symlink to map argocd to the correct binary

@kshamajain99
Copy link
Contributor

@backjo
I had a discussion with the team, argocd is planning to move out all non image binaries from help->downloads page. Our goal is to move all additional binaries to github release artifacts. So, my suggestion is that instead on building binary inside argocd image, you can build and upload to github assets in github actions release workflow.
https://github.com/argoproj/argo-cd/blob/master/.github/workflows/release.yaml

@backjo backjo closed this Mar 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants