8000 build & use multi-arch manifests for doc/crds by BlaineEXE · Pull Request #329 · k8snetworkplumbingwg/whereabouts · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

build & use multi-arch manifests for doc/crds #329

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 22, 2023
Merged

build & use multi-arch manifests for doc/crds #329

merged 1 commit into from
May 22, 2023

Conversation

BlaineEXE
Copy link
Contributor
@BlaineEXE BlaineEXE commented May 9, 2023 •
edited
Loading

Build multi-arch manifests (amd64 and arm64) for PR 'build`, 'master' images, and 'release' images. In the doc/crds/daemonset-install.yaml file, remove amd64-specific items, and use the 'latest' manifest that will autoselect the appropriate architecture for the system.

Starting as a draft PR to make sure the build test passes and to make sure to start from a place of discussion. This PR is more about convenience than necessity for me.

What this PR does / why we need it:
Allow deploying one version of the sample deployment for any (x86/arm) architecture. This simplifies deployment for users. This is a matter of convenience rather than necessity. Even users who have mixed amd64/arm64 environments can create 2 daemonsets for whereabouts -- one with the current selector for x86 and one with a modified selector for arm.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer (optional):

@BlaineEXE BlaineEXE requested review from dougbtv and maiqueb as code owners May 9, 2023 23:05
@BlaineEXE BlaineEXE marked this pull request as draft May 9, 2023 23:05
BlaineEXE
BlaineEXE commented May 9, 2023
View reviewed changes
doc/crds/daemonset-install.yaml
Comment on lines -109 to +107
image: ghcr.io/k8snetworkplumbingwg/whereabouts:latest-amd64
image: ghcr.io/k8snetworkplumbingwg/whereabouts:latest
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The core intent of the change can be boiled down to this line. Allow installing whereabouts from the example without having to do any modification work based on system architecture.

@BlaineEXE BlaineEXE force-pushed the make-master-and-release-multi-arch-manifests branch from ae773e7 to a9eb632 Compare May 9, 2023 23:35
@BlaineEXE
Copy link
Contributor Author

Metadata from the build container image step seems good to me, but I don't see all of the details that would help us be certain the build is doing the right thing. I'm going to add an additional (possibly temporary) step that will output more information about the images built.

{
    "containerimage.buildinfo/linux/amd64": {
      "frontend": "dockerfile.v0",
      "attrs": {
        "filename": "Dockerfile",
        "vcs:revision": "80a641cecef9bea7176968daf6fece4d02ebdcbf",
        "vcs:source": "https://github.com/k8snetworkplumbingwg/whereabouts"
      },
      "sources": [
        {
          "type": "docker-image",
          "ref": "docker.io/library/alpine:latest",
          "pin": "sha256:124c7d2707904eea7431fffe91522a01e5a861a624ee31d03372cc1d138a[312](https://github.com/k8snetworkplumbingwg/whereabouts/actions/runs/4931290989/jobs/8813184827?pr=329#step:5:314)6"
        },
        {
          "type": "docker-image",
          "ref": "docker.io/library/golang:1.19",
          "pin": "sha256:86af5649fa1d9265d3fe7caf633231340b93e4164b96e14bc4e1131a191c1ddd"
        }
      ]
    },
    "containerimage.buildinfo/linux/arm64": {
      "frontend": "dockerfile.v0",
      "attrs": {
        "filename": "Dockerfile",
        "vcs:revision": "80a641cecef9bea7176968daf6fece4d02ebdcbf",
        "vcs:source": "https://github.com/k8snetworkplumbingwg/whereabouts"
      },
      "sources": [
        {
          "type": "docker-image",
          "ref": "docker.io/library/alpine:latest",
          "pin": "sha256:124c7d2707904eea7431fffe91522a01e5a861a624ee31d03372cc1d138a3126"
        },
        {
          "type": "docker-image",
          "ref": "docker.io/library/golang:1.19",
          "pin": "sha256:86af5649fa1d9265d3fe7caf6332[313](https://github.com/k8snetworkplumbingwg/whereabouts/actions/runs/4931290989/jobs/8813184827?pr=329#step:5:315)40b93e4164b96e14bc4e1131a191c1ddd"
        }
      ]
    }
  }

@dougbtv
Copy link
Member
dougbtv commented May 10, 2023

Personally, I think this is a solid improvement. I'm really into it. I'd love anyone else's input, but, I'm good to move forward with this.

@BlaineEXE BlaineEXE marked this pull request as ready for review May 10, 2023 20:17
@BlaineEXE BlaineEXE force-pushed the make-master-and-release-multi-arch-manifests branch 3 times, most recently from fd7896d to 1c04325 Compare May 10, 2023 20:33
@BlaineEXE
Copy link
Contributor Author

Turns out, I'm not sure how to get more debug output b/c the docker action doesn't support load-ing multi-arch manifests for later inspection 🙄

@coveralls
Copy link
coveralls commented May 10, 2023 •
edited
Loading

Pull Request Test Coverage Report for Build 5028573781

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 72.611%
Totals Coverage Status
Change from base Build 5027942676: 0.0%
Covered Lines: 1026
Relevant Lines: 1413
💛 - Coveralls

@BlaineEXE BlaineEXE force-pushed the make-master-and-release-multi-arch-manifests branch from 1c04325 to f9d4905 Compare May 10, 2023 23:49
@maiqueb
Copy link
Collaborator
maiqueb commented May 11, 2023

The e2e fail to deploy the whereabouts daemonset.

You need to get this sorted out before the merge.

For what it's worth, conceptually, I'm OK with this PR.

@BlaineEXE BlaineEXE force-pushed the make-master-and-release-multi-arch-manifests branch 2 times, most recently from 35801bb to e564266 Compare May 15, 2023 19:05
@BlaineEXE BlaineEXE force-pushed the make-master-and-release-multi-arch-manifests branch 12 times, most recently from 7f171f5 to 4108600 Compare May 22, 2023 17:29
@BlaineEXE
build & use multi-arch manifests for doc/crds
03a7bb4 
Build multi-arch manifests (amd64 and arm64) for PR 'build`, 'master'
images, and 'release' images. In the doc/crds/daemonset-install.yaml
file, remove amd64-specific items, and use the 'latest' manifest that
will autoselect the appropriate architecture for the system.

Signed-off-by: Blaine Gardner <blaine.gardner@redhat.com>
@BlaineEXE BlaineEXE force-pushed the make-master-and-release-multi-arch-manifests branch from 4108600 to 03a7bb4 Compare May 22, 2023 17:41
@maiqueb maiqueb merged commit fbd57ab into k8snetworkplumbingwg:master May 22, 2023
@BlaineEXE BlaineEXE deleted the make-master-and-release-multi-arch-manifests branch May 22, 2023 18:57
@maiqueb
Copy link
Collaborator
maiqueb commented May 22, 2023

The multi-arch img build target failed: https://github.com/k8snetworkplumbingwg/whereabouts/actions/runs/5049399831/jobs/9058806565

Can you take a look @BlaineEXE ?
@dougbtv FYI

@jonkerj jonkerj mentioned this pull request May 25, 2023
Open
@WebberHuang1118 WebberHuang1118 mentioned this pull request Mar 13, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dougbtv dougbtv dougbtv approved these changes

@maiqueb maiqueb maiqueb approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@BlaineEXE @dougbtv @coveralls @maiqueb
0