Address review feedback from coderabbitai on
hack/e2e-apps/etcd.bats:89: the test verifies that the chart renders
EtcdBackupSchedule and that the etcd-operator materializes a CronJob,
not that backups fire. Use "0 0 1 1 *" so no Jobs run against the
intentionally invalid endpoint during the ~6-minute test window.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Address review feedback from coderabbitai on
hack/e2e-apps/etcd.bats:39-40: replace the sleep-then-wait pattern with
a wait_etcd_hr_ready helper that polls hr/etcd until the Cozystack API
reconciles the Etcd CR and only then waits for condition=ready.
kubectl wait exits immediately when the resource does not yet exist,
which caused spurious failures when HelmRelease creation took longer
than 5s.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Address review feedback from coderabbitai on
hack/e2e-apps/etcd.bats:9-14: drop the '|| true' guards in setup() so
real teardown failures (RBAC errors, stuck finalizers, wait timeouts)
fail the test setup instead of letting the next test run against stale
HelmRelease, Secret, or EtcdBackupSchedule state. --ignore-not-found
still covers the empty-namespace case on a clean run.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Address review feedback from coderabbitai on
hack/e2e-apps/etcd.bats:19-21: redirect stdout to fd 3 first, then dup
stderr onto the new fd 1 so failures from kubectl (NotFound, RBAC,
timeouts) reach the test diagnostics stream instead of being swallowed
by the bats-captured default stdout.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
cozytest.sh does not implement the bats 'run <cmd>' helper and '$status'
variable, so the 'Create Etcd with empty backup block (disabled by
default)' test died with 'run: not found' on every attempt. Replace the
bats idiom with plain shell: use '!' prefix and redirect stderr to
assert the resources absent-when-backup-disabled invariant directly.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Fourth review round fixes.
Simplify backup validation by dropping placeholder-regex guards and switching default values of s3AccessKey, s3SecretKey, destinationPath, and endpointURL from angle-bracketed hints to empty strings. A single 'required' check on each field is now the whole validation for presence; the regex guard was brittle (silently stopped matching if the hint ever changed) and dropping it makes the values.yaml defaults match the behavior contract the README describes. s3:// prefix and '//' rejection still live in etcd-backup-schedule.yaml as template-time fail branches.
Add helm-unittest suites in packages/extra/etcd/tests/ covering both templates end-to-end: rendered output assertions for the happy path (bucket/key split, region passthrough, conditional rendering of optional fields) and failedTemplate assertions for every fail branch. A 'test' target is wired into the package Makefile so hack/helm-unit-tests.sh picks it up on 'make unit-tests'.
Add type: Opaque to backup-secret.yaml for consistency with the mongodb template.
E2E: wait for HelmRelease, Secret, and EtcdBackupSchedule deletion in setup() so the empty-backup case does not flake against lingering state from the previous test. Replace the fictitious minio-e2e hostname with no-such-endpoint.invalid and explicitly document that the e2e verifies schedule/CronJob materialization, not backup execution.
Restore via EtcdCluster.spec.bootstrap and the one-shot EtcdBackup CRD are still not exposed through this chart; both are explicit follow-ups called out in the README.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
- Use name='etcd' in all three e2e cases. The chart pins the Helm
release name to 'etcd' via check-release-name.yaml, so any other
name causes template rendering to fail before the HelmRelease
can reconcile. This is why the tenant chart hardcodes HR name
'etcd' in packages/apps/tenant/templates/etcd.yaml.
- Consolidate all backup validation guards into
etcd-backup-schedule.yaml so Helm's alphabetical render order no
longer determines which failure a user sees first. Previously, a
bad destinationPath would be masked by the earlier placeholder
check in backup-secret.yaml.
- Use regex match '^<.*>$' for placeholder detection instead of
exact string equality. The previous check silently stopped
firing if anyone updated the default placeholder string in
values.yaml; the regex form catches any angle-bracketed default.
- Reject destinationPath strings containing '//' after the s3://
prefix. Previously 's3://bucket//subdir' silently produced a key
beginning with '/', confusing S3 clients and retention policies.
- Document the backup contract in README.md: what enabling it
produces, which fields are required, why placeholders are
rejected, and that restore is a known follow-up gap.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
- Mark backup.enabled as optional in the values typedef so
generated JSON schema no longer lists it in 'required'.
Previously, 'backup: {}' (relying on the enabled:false default)
failed schema validation at CRD admission.
- Validate backup.destinationPath has s3:// prefix. Without this
check, passing a path without the scheme silently used the
first path segment as the bucket name.
- Add tail newline to values.schema.json to match repo convention
(cozyvalues-gen output lacks it — worth fixing in that project
separately).
- Add bats setup() that clears tenant-test etcd resources before
each test so a failure mid-test does not leak into subsequent
tests. Hardcoded chart-level resource names ('etcd',
'etcd-s3-creds') make two Etcd apps in the same namespace
collide; setup() enforces a single owner per test.
- Add dump_diagnostics() helper that dumps operator logs + the
relevant CRD state on wait-for-ready timeouts, so CI failures
surface actionable context instead of bare exit code 124.
- Add a third e2e case 'Create Etcd with empty backup block' that
asserts backup:{} admits and produces no backup resources — a
regression test for the required:['enabled'] bug fixed here.
Known scope: the upstream EtcdCluster.spec.bootstrap restore field
(also new in v0.4.3) is not exposed through this values.yaml yet.
Restoring a cluster from an existing snapshot is a follow-up PR.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Review found three issues:
1. The guard in backup-secret.yaml only triggered on empty strings,
not on the placeholder defaults ('<your-access-key>' etc). A user
enabling backup.enabled=true without overriding the values would
produce a Secret containing the literal placeholder, and the
backup CronJob would fail at runtime with an opaque 403 far from
the source. Add explicit template guards that fail rendering when
any of the four required fields still hold their placeholder
default.
2. The backup e2e test waited on .spec.schedule of the newly-created
EtcdBackupSchedule, which is set at apply time — the wait was
tautological and did not prove the etcd-operator reconciled
anything. Replace it with a wait on the EtcdCluster's Ready
condition plus a poll for the CronJob the operator generates
from the schedule (selected via the
etcd.aenix.io/etcdbackupschedule-name label).
3. The backup e2e test also did not wait for EtcdCluster readiness
before tearing down. Add the same Ready wait as the baseline
test, and verify the Secret contains the real access key value
rather than just existing.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Adds two bats cases: a baseline Etcd managed-app deployment and a
deployment with backup.enabled=true that verifies the
EtcdBackupSchedule resource and the S3 credentials Secret are
materialized. The backup case uses fake S3 credentials — the
operator will not successfully push backups, but the schedule
resource itself is reconciled, which is what this test validates.
Also hardcodes the EtcdBackupSchedule and backup Secret names to
'etcd' / 'etcd-s3-creds' to match the existing hardcoded
EtcdCluster name in the chart (one etcd per tenant namespace).
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Aleksei Sviridkin