Hi everyone, could I get a review on this PR pleas...
# pr-reviews
i
Hi everyone, could I get a review on this PR please? It is a bug fix. • github.com/cloudposse-terraform-components/aws-eks-cert-manager/pull/67 There is a related open PR that is not authored by me that mine is complementing that might be worth checking together; • github.com/cloudposse-terraform-components/aws-eks-cert-manager/pull/66
👀 1
a
first approved and merged, not sure if the second one is ready (it had issues last week)
👍 1
i
It looks like the second one has passed
Requires merge approval now
👀 1
a
the CI tests are failing, I asked the author to take a look
👍 1
👀 1
d
I’ve updated the PR
👍 1
a
please address the 🐰 comment
d
but that’s the fix for terratest?
from my updated PR comment:
An earlier revision of this PR also replaced the deprecated
installCRDs: true
with
crds.enabled: true
+
crds.keep: true
. That broke terratest, and has been reverted — thanks to @ivan-pinatti for diagnosing it.
The test fixture pins
cert_manager_chart_version: v1.5.4
. That chart has no
values.schema.json
at all, so it does not reject the new keys — it silently ignores them. Its
templates/crds.yaml
is gated on
{{- if .Values.installCRDs }}
, so removing that key made the chart render zero CRDs. v1.5.4 also ships
startupapicheck-job.yaml
, a post-install hook running `cmctl check api --wait`; with no CRDs installed the API never becomes available, the hook hangs, and
atomic
tears the release down:
a
one more comment, if you could address it
d
it’s a variable name change because of an underlying typo. It’ll break existing installs
might be fairly minor as it defaults to
true
and I don’t think it’ll work if set to false
👍 1
a
we'll cut a major version
🎉 1
thanks @david
party parrot 1