nextcloud / helm

A community maintained helm chart for deploying Nextcloud on Kubernetes.

GNU Affero General Public License v3.0

295 stars 258 forks source link

fix: cleanup and linting of metric resources #481

Closed wrenix closed 5 months ago

wrenix commented 7 months ago

Pull Request

Additional information

Split of #478

Move the metrics-deployment related resources into dedicated folder templates/metrics. make a fee cleanups and lints for easier readable:
- indent of list
- use nindent instatt of indent
- use with instatt of if
- merge if with using and (and could handle more then two parameters)
Add namespaceSelector on servicemonitor
add ci/*-values.yaml for chart-testing

Checklist

[x] DCO has been signed off on the commit.
[x] Chart version bumped in Chart.yaml according to semver.

wrenix commented 7 months ago

Done

jessebot commented 7 months ago

Awesome, thank you! Have you tested installing this with metrics enabled?

wrenix commented 7 months ago

Nope, but if you like i will (and then i will also reimplement #349 ).

jessebot commented 7 months ago

Yes, please test parts of the code you are modifying. That's probably a good habit generally, but at least until we figure out the helm unit tests (I still haven't had the time to sit down and play with it after the most recent PR was added) or add more CI jobs to do more than the default install.

wrenix commented 7 months ago

I test it
add namespaceSelector #349

i add value files for chart-testing:


New chart version: 4.5.3
Chart version ok.
Validating /home/wrenix/src/github.com/nextcloud/helm/charts/nextcloud/Chart.yaml...
Validation success! 👍
Validating maintainers...

Linting chart with values file "charts/nextcloud/ci/ct-all-enabled-values.yaml"...

==> Linting charts/nextcloud

1 chart(s) linted, 0 chart(s) failed

Linting chart with values file "charts/nextcloud/ci/ct-specials-values.yaml"...

==> Linting charts/nextcloud

1 chart(s) linted, 0 chart(s) failed

✔︎ nextcloud => (version: "4.5.3", path: "charts/nextcloud")

All charts linted successfully

wrenix commented 7 months ago

rebased

jessebot commented 7 months ago

Thanks! Approved the workflow.

add namespaceSelector https://github.com/nextcloud/helm/pull/349

Can we put a little "Coauthored by" on a commit here to credit Richardds for the other PR so that when we merge this one, we can close #349 at the same time and say thank you?

jessebot commented 7 months ago

Please bump the chart version a minor version as we will be adding the functionality of the namespace selector here :)

wrenix commented 7 months ago

done:

rebase
new version
co-authored of @Richardds

wrenix commented 6 months ago

rebased and version dump again

wrenix commented 6 months ago

rebased and version dump again.

ping @jessebot

that is my fourth version dump, without any new review.

So, please say something or i will close this PR .... (PS: of course i could remove the ci/ct-*-values.yaml commit if we want discuss it later or still here https://github.com/nextcloud/helm/pull/480#discussion_r1427651742)

wrenix commented 5 months ago

another conflict ... - so i stop supporting this PR

provokateurin commented 5 months ago

@wrenix we are happy to review improvements, I just think that these huge PRs make it hard to review and currently nobody seems to really have time for it. If could split your changes into smaller PRs then they can be reviewed sooner.

wrenix commented 5 months ago

that is already a split into parts ... and you did not asked in this pr again ....

you could take the rebased version/commit from here: https://github.com/wrenix/nextcloud-helm/commits/main/

current it is: