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 arm64 builds #680

Merged
merged 4 commits into from Jun 9, 2022
Merged

Add arm64 builds #680

merged 4 commits into from Jun 9, 2022

Conversation

benmoss
Copy link
Contributor

@benmoss benmoss commented May 10, 2022

What this PR does / why we need it:

  • Switch to using docker buildx for building through kbld for multi-arch support

Which issue(s) this PR fixes:

Fixes #573

Does this PR introduce a user-facing change?

Adds support for arm64 in the kapp-controller image

Additional Notes for your reviewer:

Review Checklist:
  • Follows the developer guidelines
  • Relevant tests are added or updated
  • Relevant docs in this repo added or updated
  • Relevant carvel.dev docs added or updated in a separate PR and there's
    a link to that PR
  • Code is at least as readable and maintainable as it was before this
    change

Additional documentation e.g., Proposal, usage docs, etc.:


hack/dependencies.yml Outdated Show resolved Hide resolved
config/build.yml Outdated Show resolved Hide resolved
@benmoss benmoss changed the title Add arm64 builds WIP: Add arm64 builds May 10, 2022
Copy link
Contributor

@joe-kimmel-vmw joe-kimmel-vmw left a comment

Choose a reason for hiding this comment

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

i left you some annoying comments but this seems fine and I appreciate you taking it on

.github/workflows/benchmark-action.yaml Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
hack/dependencies.go Outdated Show resolved Hide resolved
@benmoss benmoss force-pushed the multi-arch branch 3 times, most recently from e5d4595 to 23f08ea Compare May 13, 2022 15:13
@benmoss benmoss marked this pull request as draft May 31, 2022 16:21
@benmoss benmoss force-pushed the multi-arch branch 4 times, most recently from afe07c7 to 38ddf13 Compare June 9, 2022 16:21
@benmoss benmoss changed the title WIP: Add arm64 builds Add arm64 builds Jun 9, 2022
@benmoss benmoss marked this pull request as ready for review June 9, 2022 16:28
@benmoss benmoss marked this pull request as draft June 9, 2022 16:52
@benmoss benmoss marked this pull request as ready for review June 9, 2022 17:12
this was from kubectl-buildkit, not needed for docker buildx
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
Copy link
Contributor

@neil-hickey neil-hickey left a comment

Choose a reason for hiding this comment

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

LGTM, you have my stamp of approval ⭐

@DennisDenuto
Copy link
Contributor

DennisDenuto commented Jun 9, 2022

Really cool stuff here!

Nice! I was wondering how you tested / iterated on this.

I am assuming that the upgrade-testing gh-action job is to test folk upgrading to the freshly cut release? If that is the case, can you update (on your fork) the upgrade-test to use the release produced on your fork? (Just to get added reassurance that the release.yaml produced in your fork results in a successful deploy)

https://github.com/vmware-tanzu/carvel-kapp-controller/blob/0726f760ad4f5685d7d9aab123d4732726e8b2cd/.github/workflows/upgrade-testing.yml#L40

Also, along this train of thought... and this suggestion probably makes way more sense to implement in a different PR, should we be adding upgrade-tests for arm deployments in addition to amd64, now that we produce images for both?

@benmoss
Copy link
Contributor Author

benmoss commented Jun 9, 2022

Nice! I was wondering how you tested / iterated on this.

The main thing I needed to do was change image_repo from config/release to my github repo's GHCR, which I did by allowing --data-values-env=KCTRL and then overriding it in my fork to just be relative to the current repo. It would be nice if this wasn't hard-coded in this repo.

Other than that just enabling GitHub actions on my fork and pushing tags is all you need to do. Just be extra paranoid that you're not pushing to this repo :). I'll usually just be explicit and git push fork develop instead of git push.

I am assuming that the upgrade-testing gh-action job is to test folk upgrading to the freshly cut release? If that is the case, can you update (on your fork) the upgrade-test to use the release produced on your fork? (Just to get added reassurance that the release.yaml produced in your fork results in a successful deploy)

Yeah, I could definitely set this up.

Also, along this train of thought... and this suggestion probably makes way more sense to implement in a different PR, should we be adding upgrade-tests for arm deployments in addition to amd64, now that we produce images for both?

Yeah, I was thinking we maybe want to run tests against an arm-deployed cluster. Will have to play around with that arm runner and see how well that works with minikube or what our other options would be.

@benmoss
Copy link
Contributor Author

benmoss commented Jun 9, 2022

Actually I don't think I understand your suggestion of running the upgrade tests from my release.. What would we be upgrading to? The test-gh and Kind Cluster E2E tests jobs test that this PR deploys successfully.

I think what you're describing is something like us generating a release.yml for every PR and just deploy/e2e that. I like the idea, I've been trying to think of how we could automate exercising our release process especially for work like this.

EIther way this build should test against my release.yml.

@DennisDenuto
Copy link
Contributor

DennisDenuto commented Jun 9, 2022

EIther way this build should test against my release.yml.

Yeah, seeing a successful deploy of the generated release.yaml based on your fork is what I was after. Alternatively a manual test using that release.yaml and seeing it succeed would also have given me that same level of confidence.

I think what you're describing is something like us generating a release.yml for every PR and just deploy/e2e that. I like the idea, I've been trying to think of how we could automate exercising our release process especially for work like this.

+1

this PR LGTM! 🚀

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.

Create arm64 image for kapp-controller
6 participants