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

fix: remove unnecessary GOARCH env in Dockerfile #1046

Merged
merged 1 commit into from
Feb 24, 2021
Merged

fix: remove unnecessary GOARCH env in Dockerfile #1046

merged 1 commit into from
Feb 24, 2021

Conversation

tydra-wang
Copy link
Contributor

What this PR does / why we need it:

When I build kic in arm64 environment, I found GOARCH hard coded in Dockerfile.

@tydra-wang tydra-wang requested a review from a team as a code owner February 24, 2021 11:17
@CLAassistant
Copy link

CLAassistant commented Feb 24, 2021

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Feb 24, 2021

Codecov Report

Merging #1046 (5ca1c95) into main (1033dee) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1046   +/-   ##
=======================================
  Coverage   49.42%   49.42%           
=======================================
  Files          32       32           
  Lines        3207     3207           
=======================================
  Hits         1585     1585           
  Misses       1492     1492           
  Partials      130      130           

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 1033dee...8e36322. Read the comment docs.

Copy link
Contributor

@shaneutt shaneutt left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! 🥳

The upstream image already supports a variety of platforms, so I can appreciate and understand wanting to remove this declaration to make it easier to build for other platforms, so from a purely functional perspective this LGTM.

However please do note that amd64 is the only platform we support at the moment and we're not yet ready to support other platforms due to testing and other logistics.

If you're interested in getting support for ARM I would recommend subscribing to #1019 and contributing there with context about your use case(s) to help drive that forward.

Copy link
Contributor

@rainest rainest left a comment

Choose a reason for hiding this comment

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

To confirm, @shaneutt your comment is effectively "we'll remove the restriction, but be aware that we don't actually support anything else and you're on your own if it breaks"?

I think that's the case, and don't see a reason we need to keep this, since we build images on amd64 machines and Go will default to that.

@shaneutt
Copy link
Contributor

To confirm, @shaneutt your comment is effectively "we'll remove the restriction, but be aware that we don't actually support anything else and you're on your own if it breaks"?

That's right.

@shaneutt shaneutt merged commit 570b107 into Kong:main Feb 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants