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

Host NFD Helm repo in gh-pages #457

Merged
merged 4 commits into from
Mar 17, 2021

Conversation

marquiz
Copy link
Contributor

@marquiz marquiz commented Feb 26, 2021

  • scripts/update-gh-pages: manage a Helm repo
    • Make the update-gh-pages.sh script to maintain a Helm charts repository
      under charts/ subdirectory in gh-pages. The script now (always) scans
      throught all release assets and injects any found Helm chart archives
      into the Helm repo. In practice, new assets in all Github releases are
      scanned and the Helm repo is updated on any update of the master or
      release branches or on any new tags. Asset ids are tracked/cached in
      order to avoid unnecessary downloads, but also, to capture any changes
      in assets that were already merged in the repo index.
    • After this a user is able to do
      $ helm repo add nfd http://kubernetes-sigs.github.io/node-feature-discovery/charts
        ...
      $ helm repo update
        ...
      $ helm install nfd/node-feature-discovery --namespace nfd --create-namespace --generate-name
        ...
      
  • scripts/prepare-release: package Helm chart
    Prepare a signed Helm chart package to be uploaded to the Github release page.
  • update release process in new-release issue template
    Capture process for Helm charts and remove some outdated bits.

I ended up this solution because:

  • gh-pages branch stays fully automated. No need for manual pushes/PRs
  • Release artefacts (Helm chart archives) can be updated after the fact. If I would've automated the creation of Helm chart archives (based on Git tags) in gh-pages this would not have been possible.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 26, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: marquiz

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

The pull request process is described 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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 26, 2021
@marquiz
Copy link
Contributor Author

marquiz commented Feb 26, 2021

Needs some more testing...
/hold

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Feb 26, 2021
@marquiz marquiz force-pushed the devel/helm-repo branch 6 times, most recently from 7a52c5e to b4f0b2d Compare February 26, 2021 15:43
@marquiz
Copy link
Contributor Author

marquiz commented Feb 26, 2021

This deployment using my personal Helm repo works:

$ helm repo add nfd https://marquiz.github.io/node-feature-discovery/charts

$ helm repo update

$ helm install nfd/node-feature-discovery --namespace nfd --create-namespace --generate-name

It has v0.7.0 chart generated by the prepare-release.sh and the charts repo updated by update-gh-pages.sh. Related Github workflow log:
https://github.com/marquiz/node-feature-discovery/runs/1988617946?check_suite_focus=true

I think this is ready for review. It'd be good if someone more acquainted with Helm than me (e.g. @adrianchiris) would take a look 😉
/unhold

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 26, 2021
@marquiz marquiz changed the title Host NFD Helm repo Host NFD Helm repo in gh-pages Feb 26, 2021
@adrianchiris
Copy link
Contributor

Awesome @marquiz ! i will definitely dig into this PR.

Copy link
Contributor

@adrianchiris adrianchiris left a comment

Choose a reason for hiding this comment

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

Hi there @marquiz ! Sorry for the late response.

Overall, i think this change is good.
A couple of (naive) comments.

my next step is to fork and play around with this change.

scripts/prepare-release.sh Outdated Show resolved Hide resolved
scripts/github/update-gh-pages.sh Show resolved Hide resolved
Make the update-gh-pages.sh script to maintain a Helm charts repository
under charts/ subdirectory in gh-pages. The script now (always) scans
throught all release assets and injects any found Helm chart archives
into the Helm repo. In practice, new assets in all Github releases are
scanned and the Helm repo is updated on any update of the master or
release branches or on any new tags. Asset ids are tracked/cached in
order to avoid unnecessary downloads, but also, to capture any changes
in assets that were already merged in the repo index.

After this a user is able to do something like

$ helm repo add nfd http://kubernetes-sigs.github.io/node-feature-discovery/charts
  ...
$ helm repo update
  ...
$ helm install nfd/node-feature-discovery --namespace nfd --create-namespace --generate-name
  ...
@marquiz marquiz force-pushed the devel/helm-repo branch 2 times, most recently from 8f4b3ef to 88a1613 Compare March 9, 2021 13:45
Prepare a signed Helm chart package to be uploaded to the Github release
page.
Capture process for Helm charts and remove some outdated bits.
@marquiz marquiz mentioned this pull request Mar 12, 2021
32 tasks
@marquiz
Copy link
Contributor Author

marquiz commented Mar 12, 2021

@adrianchiris have you had the time to look at this?

@adrianchiris
Copy link
Contributor

not since our last slack discussion, will get to it this week.

@marquiz
Copy link
Contributor Author

marquiz commented Mar 16, 2021

not since our last slack discussion, will get to it this week.

That would be great as this is basically the last open PR blocking the release

Copy link
Contributor

@adrianchiris adrianchiris left a comment

Choose a reason for hiding this comment

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

OK, not a bash expert here but the code seems OK to me.

i have also tested the changes by doing a dummy release on my forked repo ensuring the chart can be downloaded and is of the correct version.

I wasnt able to verify the package with provenance file unfortunately, but that may be related to my gpg setup and helm quirks.

@marquiz were you able do do a helm verify on the package ?

other than that im lgtm on this.

@marquiz
Copy link
Contributor Author

marquiz commented Mar 16, 2021

I wasnt able to verify the package with provenance file unfortunately, but that may be related to my gpg setup and helm quirks.

@marquiz were you able do do a helm verify on the package ?

Thanks for the review @adrianchiris! I thought I correctly verified the --verify option but must have done that locally with incorrect archive or fatfingered in some other way because now it failed 😬 The root cause for this is the filename mangling we do in the release script. I pushed a new patch constructing the provenance file by and and using gpg for signing it. Improves user experience 😄
Haven't yet verified that, though. Will report back when I have

Need to create the provenance file by hand as we mangle the name of the
chart archive. However, this also provides better user experience (for
the release manager) as gpg version 2.1 and later are supported.
Copy link
Contributor

@adrianchiris adrianchiris left a comment

Choose a reason for hiding this comment

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

pulled the change,

after creating the chart package:

helm verify ./node-feature-discovery-chart-0.8.0.tgz
Signed by: Adrian Chiris <adrianc@nvidia.com>
Using Key With Fingerprint: 7E3B1541735B7D751CD2BB1BCC911D9D88FC463E
Chart Hash Verified: sha256:08932a483344cb5e3ef501083253fc8ab7bfe00f0463e6c06a2fe1bfa2851abd

@marquiz
Copy link
Contributor Author

marquiz commented Mar 17, 2021

Yeah, I also verified this locally. Not with install --verify yet

@marquiz
Copy link
Contributor Author

marquiz commented Mar 17, 2021

@adrianchiris verified install from my personal repo now works:

$ helm install --generate-name --namespace nfd-helm --create-namespace --verify nfd/node-feature-discovery
NAME: node-feature-discovery-1615973606
LAST DEPLOYED: Wed Mar 17 11:33:29 2021
NAMESPACE: nfd-helm
STATUS: deployed
REVISION: 1
TEST SUITE: None

@adrianchiris
Copy link
Contributor

Awsome !

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 17, 2021
@k8s-ci-robot k8s-ci-robot merged commit 086f8c3 into kubernetes-sigs:master Mar 17, 2021
@marquiz marquiz deleted the devel/helm-repo branch March 17, 2021 12:38
@kfox1111
Copy link

Awesome. :)

Can someone add a reference to the repo to artifacthub.io please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants