Add option to ignore PVCs without annotation

Stogas commented 4 months ago

Summary

Add CLI flag / environment variable to allow overriding the default behavior for PVCs without k8up.io/backup annotation
Allow setting this variable through Helm chart's values
Add relevant notes in documentation, where seems applicable

Relates to https://github.com/k8up-io/k8up/issues/550

Checklist

For Code changes

[ ] Categorize the PR by setting a good title and adding one of the labels: bug, enhancement, documentation, change, breaking, dependency as they show up in the changelog
[ ] PR contains the label area:operator
[ ] Link this PR to related issues
[ ] I have not made any changes in the charts/ directory.

For Helm Chart changes

[ ] Categorize the PR by setting a good title and adding one of the labels: bug, enhancement, documentation, change, breaking, dependency as they show up in the changelog
[ ] PR contains the label area:chart
[ ] PR contains the chart label, e.g. chart:k8up
[ ] Variables are documented in the values.yaml using the format required by Helm-Docs.
[ ] Chart Version bumped if immediate release after merging is planned
[ ] I have run make chart-docs
[ ] Link this PR to related code release or other issues.

Stogas commented 4 months ago

btw, I am unable to set any labels on this issue

Stogas commented 4 months ago

I can see some chart tests are failing, partially because I haven't updated the chart tests to handle the new variable.

I'd love to help, but would love some guidance, as I don't have much experience with those.

Kidswiss commented 4 months ago

Hi @Stogas

Thanks for the contribution! Looks like a useful feature!

I can see some chart tests are failing, partially because I haven't updated the chart tests to handle the new variable.

I'd love to help, but would love some guidance, as I don't have much experience with those.

The lint job is failing because the docs haven't been updated. You can run make chart-docs to fix that.

~As for the other test: I'd need to dig into that one a bit as well. I haven't had those fail on me before. I think we just need to specify the new env var somewhere, so the test is aware about it.~

EDIT: okay it's not black magic :D the test basically looks at hardcoded positions in the envVars of the rendered chart. By adding a new one, things probably moved around a bit. See here: https://github.com/k8up-io/k8up/blob/master/charts/k8up/test/deployment_test.go#L40

Stogas commented 4 months ago

Thanks for the quick reply!

I've added a fix for both, so both should succeed. However, I'm not entirely sure how to add and check for the value of the envVar in the generated manifest, so I added a test to only look for the envVar name in the right location.

Kidswiss commented 4 months ago

Hmmm... still isn't quite happy. Btw you can use make chart-test you can run that test locally.

I can have a look later how to properly add your new envVar.

Stogas commented 4 months ago

@Kidswiss yeah, I'm not sure why the fix didn't help.

Let me know if I can help somehow!

Stogas commented 4 months ago

@Kidswiss fixed, requested re-review

k8up-io / k8up