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

Helm/refactor nifi cluster #341

Merged
merged 11 commits into from
Jan 3, 2024

Conversation

umarhussain15
Copy link
Contributor

@umarhussain15 umarhussain15 commented Dec 20, 2023

Q A
Bug fix? no
New feature? no
API breaks? no
Deprecations? no
Related tickets
License Apache 2.0

What's in this PR?

This PR is mostly refactoring the helm chart nifi-cluster.

  • It removes the template files under helm/nifi-cluster/charts/zookeeper since we get the zookeeper chart via dependencies from Chart.yaml and that will be packaged with nifi-cluster.
  • The webProxyHosts value substitution in nifi-cluster.yaml is fixed to convert it to yaml so it is yaml array instead of spaced values. The default value is fixed to array in values.yaml as well.
  • The nifi-cluster.yaml is refactored to remove extra lines in the output.
  • Documentation links in values.yaml updated to correct one. More comments added. And the chart's README is regenerated (Maybe at some point, add the option to Makefile to generate it?)
  • Warning added for SSL certificate generation via cert-manager to indicate that hostname should be at max 64 bytes.

Checklist

  • Implementation tested
  • Error handling code meets the guideline
  • Logging code meets the guideline
  • User guide and development docs updated (if needed)
  • Append changelog with changes

@mh013370
Copy link
Member

mh013370 commented Dec 21, 2023

We should also update this zookeeper chart dependency. It's 2 major versions behind.

We're running with the latest bitnami zookeeper docker image, so i know that's safe but perhaps we should also update the chart dependency?

https://artifacthub.io/packages/helm/bitnami/zookeeper

@umarhussain15 umarhussain15 force-pushed the helm/refactor-nifi-cluster branch from 651c5d6 to c9bdfa2 Compare December 21, 2023 11:38
@umarhussain15
Copy link
Contributor Author

I have added helm-chart-version-match in Makefile which is now called with test and test-with-vendor. It will fail if there is a mismatch between helm chart version value under /helm directory

@mh013370
Copy link
Member

any concerns @juldrixx ?

@juldrixx juldrixx requested review from juldrixx and erdrix December 23, 2023 10:25
Copy link
Contributor

@juldrixx juldrixx left a comment

Choose a reason for hiding this comment

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

any concerns @juldrixx ?

Yes, I don't think that a good idea. In company, like mine, where we use mirror as Artifactory to access public repository. The chart won't work because we will be unable to pull it. Because it won't be mirrored.

@umarhussain15
Copy link
Contributor Author

Hi @juldrixx,
the zookeeper chart will be packaged inside the nifi-cluster chart. Running make helm-pacakge in the pipeline generates the tgz file in which the under charts folder the zookeeper template files are present.
So access to bitnami chart repo will only be required during pipeline generating the helm chart package. Users of the nifi-cluster will only need access to oci://ghcr.io/konpyutaika/helm-charts.

You can check the content of the packaged helm chart for this PR:

  • make helm-pacakge generates nifi-cluster-1.6.0.tgz
  • tar -tvf nifi-cluster-1.6.0.tgz --wildcards 'nifi-cluster/charts/zookeeper/*' lists all the templates present for zookeeper

@mh013370
Copy link
Member

So this is effectively just removing it from version control, which i'm on board with and it simplifies maintenance of the dependent chart!

@mh013370
Copy link
Member

mh013370 commented Jan 3, 2024

Okay, this all looks good to me. I think @juldrixx 's concern is addressed: The zookeeper chart is still bundled (for airgap purposes), but the bundling is performed by make at release time and will not be maintained in version control from now on.

@mh013370
Copy link
Member

mh013370 commented Jan 3, 2024

@umarhussain15 : Can you please update the CHANGELOG with the changes/additions you've made?

The `helm dependency update` will pull the dependency chart based on `Chart.yaml` so we don't need it as templated files

Update gitignore for tgz files under charts/

Signed-off-by: Umar Hussain <[email protected]>
…yaml so that array is applied correctly

Signed-off-by: Umar Hussain <[email protected]>
… The chart version now matches the operator chart version

Signed-off-by: Umar Hussain <[email protected]>
…n and helm chart dependency update to makefile

Add helm chart version match to `test` and `test-with-vendor` to fail if one of the chart has different version

Signed-off-by: Umar Hussain <[email protected]>
@umarhussain15 umarhussain15 force-pushed the helm/refactor-nifi-cluster branch from 50cfbdf to eb84a49 Compare January 3, 2024 12:06
@mh013370 mh013370 merged commit 15c9e78 into konpyutaika:master Jan 3, 2024
5 checks passed
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.

3 participants