-
Notifications
You must be signed in to change notification settings - Fork 17
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
feat(*): add node installer dir and workflow #26
Conversation
nit: consider renaming |
@Mossaka agreed it might be better to use a different name. Slightly hesitant to use |
Would it be possible to generate the node installer image on PRs as well? |
We could; working on a re-org to support this. As a follow-up in the future, if we wanted, we could also push the k3d img on PRs while we are at it. |
kwasm-node-installer/Dockerfile
Outdated
|
||
RUN mkdir -p /release/bin/ \ | ||
&& curl -L https://github.com/containerd/runwasi/releases/download/containerd-shim-wasmedge%2Fv0.3.0/containerd-shim-wasmedge-$(uname -m | sed s/arm64/aarch64/g | sed s/amd64/x86_64/g).tar.gz | tar -xzf - -C /release/bin/ \ | ||
&& curl -L https://github.com/containerd/runwasi/releases/download/containerd-shim-wasmtime%2Fv0.3.0/containerd-shim-wasmtime-$(uname -m | sed s/arm64/aarch64/g | sed s/amd64/x86_64/g).tar.gz | tar -xzf - -C /release/bin/ \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One point I want to raise here is — this is the custom node installer image for SpinKube and the Spin operator project, and we generate this image from the Spin shim.
I'd like to propse that this image only configures the Spin shim, for a number of reasons:
- the other shims are not used by SpinKube
- we are not updating them as part of this installer
- we might actually overwrite someone's desired other shim version
- they represent a security vulnerability area that is unnecessary because of the first two points
Managing other shims and runtime classes is the job for https://github.com/spinkube/runtime-class-manager/, and configuration there IMO is where someone would decide to bring in another shim.
Signed-off-by: Vaughn Dice <[email protected]>
Signed-off-by: Vaughn Dice <[email protected]>
e0eefe7
to
ca85084
Compare
In the latest commits:
|
Signed-off-by: Vaughn Dice <[email protected]>
Looks like the workflow can't push the node-installer image, probably due ro running from a PR. Is there a setting we can toggle to allow publishing packages from PRs? Or perhaps I need to create the branch on origin and not my fork?
Edit: Yes, images (packages) can only be published when the PR branch is on origin. (See #30 and https://github.com/spinkube/containerd-shim-spin/actions/runs/8191813640/job/22403982746?pr=30). I assume that's our desired state as opposed to allowing PRs from forks to publish packages - if even possible. Hence, I've gated the job appropriately in 198e74f. cc @radu-matei Edit 2: Actually, my attempt at gating the job if a PR is from a fork isn't working as expected; looking... |
68f8593
to
198e74f
Compare
Yeah |
Edit: wait, trying one more thing... yup 167ab94 works to only push the image if PR branch is on origin. |
Signed-off-by: Vaughn Dice <[email protected]>
198e74f
to
167ab94
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
uses: ./.github/workflows/node-installer.yaml | ||
needs: build | ||
# This action requires use of the GITHUB_TOKEN to publish the image | ||
# By default, PRs from forks don't have access, so we only run when the PR branch is on origin. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this imply that maintainers of this project should push feat branches to origin?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Should I add a mention of this in other areas as well? Maybe the README?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's necessary to put it in the README
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
refs/heads/main
where the 10 char split breaks)I believe the intention is to eventually have spinkube/runtime-class-manager produce this image via fetching versioned shim releases from this repo, but this gets us an image with the latest shim version that we/users can plug in when installing the kwasm-operator chart, near-term.
Tested on fork eg https://github.com/vdice/containerd-shim-spin/actions/runs/8181643173