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

Feature/add helm chart #800

Merged
merged 50 commits into from
Feb 21, 2022
Merged

Feature/add helm chart #800

merged 50 commits into from
Feb 21, 2022

Conversation

JorisHeadease
Copy link
Contributor

Helm Chart for nuts-node

This PR eases the process of deploying the nuts-node on a Kubernetes cluster. It contains the following:

  1. A new folder ./charts which contains all of the Helm-related files. See ./charts/nuts-node/README.md for more information on how it works.
  2. A new GitHub workflow ./.github/workflows/helm-chart-release.yaml. This workflow will create new releases when the version of ./charts/nuts-node/Chart.yaml changes. The new version is also published to the index.yaml of the gh-pages branch. This branch should be selected in the GitHub pages config. When published, the pages URL can be used as a Helm repo (helm repo add nuts-repo https://nuts-foundation.github.io/nuts-node/)

TODO before merging PR

This PR depends on having a gh-pages branch. See my fork as an example. The index.yaml will be created automatically. The other files can be copied but are optional.

After creating the branch, GitHub pages should be activated for this nuts-node repository, based on this gh-pages branch.

##Discussion

When to publish a Chart?

The current CI setup will trigger only on newly created tags. Therefore, it's important that the versions in the ./charts/nuts-node/Chart.yaml are updated before creating the tag, as this file will be used to determine the version for the Helm chart push.

Another option is to check if a new version should be pushed to the Helm repo on every commit to master. A new version would be a manual update of the ./charts/nuts-node/Chart.yaml. When multiple pushes occur with the same versions in the ./charts/nuts-node/Chart.yaml, the initial version remains.

Ideally, the above is automated with a release action in the future.

JorisHeadease and others added 30 commits January 20, 2022 13:43
…mit:"663a896f4a815053445eec4153677ddc24a0a361", GitTreeState:"clean", GoVersion:"go1.17.3"})
…g the keys onto the filesystem. Enabling ssl to verify test the functionality. Also clarified the config path for the nuts data PV and made the nuts config file read-only.
…ork in local IDE but hoping it does in GitHub
…sions yet, once this is present this will need to be set, and we can publish the releases to the NUTS Helm repo once added.
Upping the chart version. Testing if the CI pushes the new release
Mimicking an appVersion update
JorisHeadease and others added 6 commits January 31, 2022 11:20
Mimicking another appVersion update, the previous version was pushed to the helm repo index.yaml already with the testing of a "tag" publish
testing new chart push
…d the tags as this is already processed by a push to the master branch.
Copy link
Member

@woutslakhorst woutslakhorst left a comment

Choose a reason for hiding this comment

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

My initial thought would be to place everything under a helm directory . Then when additional scripts arrive for terraform/k8s we can move everything under deploy/X.

I created a gh-pages branch, it's configured and ready to deploy (waiting for some kind of trigger?)

In time the README can be moved to the docs dir. (When everything is setup and running).

Good enough for now, be aware of the comments.

# incremented each time you make changes to the application. Versions are not expected to
# follow Semantic Versioning. They should reflect the version the application is using.
# It is recommended to use it with quotes.
appVersion: "1.16.3"
Copy link
Member

Choose a reason for hiding this comment

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

chosen randomly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I used the default Helm value and upped a view times to test triggering an update for CI-purposes. As nuts doesn't have a version yet, I didn't set any specific version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both changed to 0.0.1

readOnly: true
- name: nuts-data-pv
mountPath: /opt/nuts/data
ports:
Copy link
Member

Choose a reason for hiding this comment

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

all ports and probe settings use the default. When settings change, these will have to change as well. It's either a todo for the future or something to pickup now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! I've actually tried modifying this before, but the config seems to contain address props, and no separate port numbers.

Do you have a preferred solution? Do the config values always end with :<port_number>? Or can they also simply be hostnames? Assuming it should be based on the following two properties:

http.default.address                       :1323
network.grpcaddr                           :5555

Copy link
Member

Choose a reason for hiding this comment

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

You will most certainly see IP's in the config because of being able to bind to specific interfaces (separate internal IPs from external ones). But I don't know if that is a situation for k8s. The pod will only have a single IP and the ingress will handle anything coming from outside? You could therefore make the choice to limit these settings to ports only. (and document it)?

JorisHeadease and others added 9 commits February 3, 2022 10:48
upping the version to test a CI build from a tag
Mimicking release v1.16.5
… in adding the config property or with releasing via tags
…so upping the appVersion to see if this affects how chart version differences are calculated.
…there are issues with building from tags (helm/chart-releaser-action#96). Adding this feature branch for testing purposes. Also added back the `CR_CHARTS_DIR` prop as it was unrelated to the bug in the CI action.

Reverting the chart push trigger to changes in the build. Apparently there are issues with building from tags (helm/chart-releaser-action#96). Adding this feature branch for testing purposes.
Upping version to test chart release via branch change instead of tag
@JorisHeadease
Copy link
Contributor Author

Thanks for the review @woutslakhorst!

We could move everything into another directory. This would require some additional configuration for the GitHub chart-releaser action. I've just committed this config property (CR_CHARTS_DIR) with the default value. That should ease the process of moving it around.

I've changed the "CI trigger" from tag version to any commit on the master branch due to a bug in the GitHub action. This does mean whenever the branch is merged, it will directly create a Helm chart. So we'd have to use reasonable version numbers. I've changed this to the following:

# This is the chart version. This version number should be incremented each time you make changes
# to the chart and its templates, including the app version.
# Versions are expected to follow Semantic Versioning (https://semver.org/)
version: 0.0.1

# This is the version number of the application being deployed. This version number should be
# incremented each time you make changes to the application. Versions are not expected to
# follow Semantic Versioning. They should reflect the version the application is using.
# It is recommended to use it with quotes.
appVersion: "0.0.1"

@woutslakhorst
Copy link
Member

@JorisHeadease do you want to make the ports configurable or leave it as is for now?

@JorisHeadease
Copy link
Contributor Author

I've just added a commit to make the properties configurable @woutslakhorst.

@woutslakhorst
Copy link
Member

@JorisHeadease will merge on thumbsup

@woutslakhorst woutslakhorst merged commit 7b07124 into nuts-foundation:master Feb 21, 2022
@JorisHeadease JorisHeadease deleted the feature/add-helm-chart branch February 22, 2022 08:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants