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

Docker Multiple Architecture Support :fixes#9089 #132

Merged
merged 5 commits into from
Sep 2, 2022

Conversation

mohammedtabish0
Copy link
Contributor

@mohammedtabish0 mohammedtabish0 commented Aug 31, 2022

resolves apache/skywalking#9089

  • Added feature to create docker multiple architectures images (linux/amd64 and linux/arm64)

@wu-sheng wu-sheng requested review from fgksgf and kezhenxu94 August 31, 2022 14:21
@mohammedtabish0
Copy link
Contributor Author

@kezhenxu94
Haven't added > /dev/null 2>&1 and || true so that command is runnable in other platforms like windows(do we really need it?). But will cause issue if builder gets created and buildx command gets failed then running again will throw error's like builder already present.

@kezhenxu94
Copy link
Member

@kezhenxu94

Haven't added > /dev/null 2>&1 and || true so that command is runnable in other platforms like windows(do we really need it?).

I doubt whether it's runnable on Windows even you don't add that. Developers usually run this kind of commands in a Windows Linux Subsystem where > /dev/null 2>&1 and || true are supported. I just think cleaning the builder(and not to report issue in second run) is more important than running on Windows (I believe rare developers run this in a bare Windows terminal like cmd or Powershell. WDYT?

@kezhenxu94 kezhenxu94 added this to the 0.5.0 milestone Sep 1, 2022
@mohammedtabish0
Copy link
Contributor Author

@kezhenxu94 Agreed, added these commands.
Also, I have made it different from skywalking's makefile as IMO the || true should only be in the buildx command because if this command gets failed then cleanup of builder should run and if we are unable to create builder then we should not proceed further.

Makefile Outdated Show resolved Hide resolved
Copy link
Member

@kezhenxu94 kezhenxu94 left a comment

Choose a reason for hiding this comment

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

@mohammedtabish0 thank you for your patience and hard work!

@kezhenxu94 kezhenxu94 merged commit cbb6f55 into apache:main Sep 2, 2022
@mohammedtabish0 mohammedtabish0 deleted the feature/multiarch-support branch September 2, 2022 02:25
@mohammedtabish0
Copy link
Contributor Author

@kezhenxu94 This was really fun, although I changed only three lines but got to learn a lot. Thank you so much for the patience and guidance. Hope to contribute more to the projects.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[INFRA/eyes] Provide multi-architecture Docker images
3 participants