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

Added helm package oci registry support #1282

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

supu2
Copy link

@supu2 supu2 commented Jan 11, 2025

Is this a BUG FIX or a FEATURE ?:
I've added helm package and oci registry support. People who don't want to use olm they can use helm.
/kind enhancement

@kubevirt-bot
Copy link
Collaborator

Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@kubevirt-bot kubevirt-bot added dco-signoff: no Indicates the PR's author has not DCO signed all their commits. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jan 11, 2025
@kubevirt-bot
Copy link
Collaborator

Hi @supu2. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Comment on lines 53 to 56
- name: Setup Go 1.21.x
uses: actions/setup-go@v5
with:
go-version: '1.21.x'
Copy link
Member

Choose a reason for hiding this comment

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

Can you install the go version from go.mod ?

deploy/helm/Chart.yaml Show resolved Hide resolved
deploy/helm/Chart.yaml Show resolved Hide resolved
make manifests
helm dependency update ./${{ env.CHART_DIR }}
helm package ./${{ env.CHART_DIR }} --destination ./charts --app-version ${{ steps.bump-semver.outputs.new_version }} --version ${{ steps.bump-semver.outputs.new_version }}
helm push ./charts/${{ github.event.repository.name }}-${{ steps.bump-semver.outputs.new_version }}.tgz oci://ghcr.io/${{ github.repository_owner }}/${{ github.event.repository.name }}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
helm push ./charts/${{ github.event.repository.name }}-${{ steps.bump-semver.outputs.new_version }}.tgz oci://ghcr.io/${{ github.repository_owner }}/${{ github.event.repository.name }}
helm push ./charts/${{ github.event.repository.name }}-${{ steps.bump-semver.outputs.new_version }}.tgz oci://${{ env.REGISTRY }}/${{ github.repository_owner }}/${{ github.event.repository.name }}

@@ -21,6 +21,9 @@ on:
- release-0.64
- release-0.52
- release-0.47
env:
REGISTRY: ghcr.io
Copy link
Member

Choose a reason for hiding this comment

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

this is for Helms repository not for kubernetes-nmstate containers.

Suggested change
REGISTRY: ghcr.io
HELM_REGISTRY: ghcr.io

@kubevirt-bot kubevirt-bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. and removed dco-signoff: no Indicates the PR's author has not DCO signed all their commits. labels Jan 25, 2025
@kubevirt-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from qinqon. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubevirt-bot kubevirt-bot added dco-signoff: no Indicates the PR's author has not DCO signed all their commits. and removed dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Jan 26, 2025
@@ -0,0 +1,6 @@
apiVersion: v2
Copy link
Member

Choose a reason for hiding this comment

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

A bunch of nits for the manifest. If we are going to build&push it to the public, let's have metadata complete.

Comparing with e.g. https://github.com/kubernetes/ingress-nginx/blob/main/charts/ingress-nginx/Chart.yaml and looking at our current https://github.com/nmstate/kubernetes-nmstate/blob/main/deploy/bases/kubernetes-nmstate-operator.clusterserviceversion.yaml, what we can easily add is

  • home
  • icon
  • keywords
  • kubeVersion
  • maintainers
  • sources

Copy link
Author

Choose a reason for hiding this comment

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

@mkowalski updated chart file. I'm not sure kubeversion support I hope nmstate works on 1.21.0

Signed-off-by: Supu <[email protected]>
@kubevirt-bot kubevirt-bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. and removed dco-signoff: no Indicates the PR's author has not DCO signed all their commits. labels Jan 30, 2025
maintainers:
- email: [email protected]
name: Enrique Llorente Pastora
- email: [email protected]
Copy link
Member

Choose a reason for hiding this comment

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

@mkowalski would you mind if you were listed instead of me? It would better reflect the reality

Copy link
Member

Choose a reason for hiding this comment

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

This will happen. It's just that it's not the only place where we should remove you (e.g. there is also the OLM manifest) so I would merge this one as-is and then I will handle the metadata update myself

Copy link
Member

Choose a reason for hiding this comment

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

works for me

@@ -0,0 +1 @@
../../../build/_output/manifests/nmstate.io_v1_nmstate_cr.yaml
Copy link
Member

Choose a reason for hiding this comment

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

Those are all dangling symlinks, i.e. when you just git clone they will not point to existing files, not even to the directory structure.

I would rather keep deploy/ in the repository clean and have something like

mkdir -p deploy/helm/templates
cp ../../../build/_output/manifests/* deploy/helm/templates/

inside the pipeline, so after make manifests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dco-signoff: yes Indicates the PR's author has DCO signed all their commits. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. kind/enhancement size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants