Add option to skip CRDs in a Release #114
Conversation
|
Hi @turkenh, I might need your approval for the PR-related GH workflow run, as this is my first-time contribution, thank you! |
|
@turkenh Looks like
I can try to increase the timeout in |
Just retriggered and worked fine this time. |
There was a problem hiding this comment.
Thanks a lot for your contribution @somaritane!
Looking good, just added two nits.
| repository: https://charts.bitnami.com/bitnami | ||
| version: 2.0.12 | ||
| namespace: argo-cd | ||
| skipCRDs: true |
There was a problem hiding this comment.
Instead of new example yaml, please add the optional config as commented out like this.
There was a problem hiding this comment.
I was considering that option from the beginning. But the problem with wordpress chart is that it doesn't install CRDs. As result, spec.forProvider.skipCRDs won't have any effect on deployment, and the example won't be true to life.
That's why I've added a separate example with argo-cd chart.
But sure, I can amend.
There was a problem hiding this comment.
I see, good point.
I would still not be too worried given this file, esp. commented-out lines, are usually just to show available parameters.
Maybe we can just use argo-cd in the original release example, wdyt?
There was a problem hiding this comment.
Agree, amended. Using argo-cd helm as the original release example would require extra efforts on mapping all parameter examples to real-world argo-cd deployment cases, might be an extra enhancement outside of this PR :)
pkg/clients/helm/client.go
Outdated
| @@ -74,7 +74,7 @@ type client struct { | |||
| } | |||
|
|
|||
| // NewClient returns a new Helm Client with provided config | |||
| func NewClient(log logging.Logger, config *rest.Config, namespace string, wait bool, timeout time.Duration) (Client, error) { | |||
| func NewClient(log logging.Logger, config *rest.Config, namespace string, wait bool, timeout time.Duration, skipCRDs bool) (Client, error) { | |||
There was a problem hiding this comment.
Nit:
With the number of parameters increasing in the function signature here, I believe we should switch using variadic functional options.
Signed-off-by: Timofey Ilinykh <[email protected]>
somaritane
force-pushed
the
skip-crds-in-release
branch
from
dd2880a
to
0e533c6
Compare
Signed-off-by: Timofey Ilinykh [email protected]
Description of your changes
This PR adds the ability to skip the installation of CRDs for a Release.
It is configured by
spec.forProvider.skipCRDsfield (=falseby default).The effect is equal to Helm's
--skip-crdsoptionFixes #107
I have:
make reviewableto ensure this PR is ready for review.How has this code been tested
Tested against local dev cluster using the example chart with CRD templates (examples/sample/release-with-crds.yaml).