Conversation
Signed-off-by: Ian Miller <milleryan2003@gmail.com>
Signed-off-by: Ian Miller <milleryan2003@gmail.com>
Signed-off-by: Ian Miller <milleryan2003@gmail.com>
Signed-off-by: Ian Miller <milleryan2003@gmail.com>
Signed-off-by: Ian Miller <milleryan2003@gmail.com>
rubambiza
left a comment
There was a problem hiding this comment.
Excellent work — this is a thorough E2E suite with great documentation (the architecture section in the README is particularly good). CI is green including the actual E2E run at ~11 min. A few items to address:
Should fix:
- Pin
azure/setup-helm@v4to a SHA. Using a major version tag means any upstream change takes effect immediately without review. Other kagenti repos pin action SHAs for this reason. Please replace with a pinned SHA, e.g.:You can find the current SHA withuses: azure/setup-helm@b9e51907a09c216f16ebe8536097933489208112 # v4.3.0
gh api repos/azure/setup-helm/git/ref/tags/v4.3.0 --jq '.object.sha'(or whichever version you want to pin).
Minor / non-blocking:
-
Copyright year in
fixtures.gosaysCopyright 2025— should be 2026. -
Hardcoded
python:3.11-slimin fixtures gets pulled from Docker Hub on every run. Docker Hub rate limits could cause flaky failures in CI. Not blocking, but worth noting for future resilience (e.g., a pre-pulled image or a registry mirror). -
Consistentlywindow for "no protocol label" is 15s with 5s polling — only 3 checks. If the controller is slow to reconcile, this could produce false passes. Consider widening to 30s or reducing the polling interval. -
Controller deployed twice — both
ManagerandAgentCard E2EDescribe blocks callDeployController()/UndeployController(). Works correctly but adds ~2-3 min to the suite. Could share the deployment if runtime becomes a concern.
kevincogan
left a comment
There was a problem hiding this comment.
Solid E2E suite. The 6 AgentCard scenarios are well-designed, semantically correct against the actual controller/webhook behavior, and the documentation is excellent. A few items need addressing before merge.
Must fix:
DeployControllerswallows all namespace creation errors. See inline comment onutils.go.appendaliasing bug inPatchControllerArgs. See inline comment onutils.go.
Should fix:
KubectlApplyStdinduplicatesutils.Runinternals. See inline comment onutils.go.
Missing test:
- Controller arg restoration verification. After
RestoreControllerArgsruns in eachAfterAll, there is no assertion that the deployment is actually running with the original args. A read-back check would catch theappendaliasing bug if the fix is not applied correctly. Worth adding in this PR since it is only a few lines.
Items 3-4 are straightforward additions that would be good to land in the same PR.
…gent cards Signed-off-by: Ian Miller <milleryan2003@gmail.com>
Signed-off-by: Ian Miller <milleryan2003@gmail.com>
kevincogan
left a comment
There was a problem hiding this comment.
LGTM, all review findings addressed. Clean implementation. Thanks! :))
Summary
This PR is responsible for:
audit mode, and SPIRE signature verification
workflow updates
Test scenarios
spec.targetRefprotocol.kagenti.io/*Related issue(s)
RHAIENG-3717
(Optional) Testing Instructions
kind delete cluster && kind create cluster && make test-e2e