Add e2e cert-manager tests from upstream Kueue#1512
Add e2e cert-manager tests from upstream Kueue#1512openshift-merge-bot[bot] merged 2 commits intoopenshift:mainfrom
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: MaysaMacedo The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a Makefile target to run upstream cert-manager e2e tests; updates e2e patch to create namespaces via Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Makefile (1)
247-256:oc createis not idempotent — useoc applyfor CI resilience.Both
oc create -f hack/manifests/jobset-operator.yamlandoc create -f hack/manifests/jobset-operand.yamlwill exit non-zero with an "already exists" error on any retry (e.g., a failed-and-retried CI job or a partial re-run). Replacing withoc applymakes the target safe to re-execute, consistent with howdeploy-ocp(line 88) handles its manifests.Note:
install-lws-operatorhas the same pattern and would benefit from the same change.♻️ Proposed fix
- oc create -f hack/manifests/jobset-operator.yaml + oc apply -f hack/manifests/jobset-operator.yaml `@echo` "Waiting for Jobset Operator to be installed" `@timeout` 300s bash -c 'until oc get deployment jobset-operator -n openshift-jobset-operator -o jsonpath="{.status.conditions[?(@.type==\"Available\")].status}" | grep -q "True"; do sleep 10; echo "Still waiting..."; done' `@echo` "Jobset Operator installed" `@echo` "Creating Jobset Instance" - oc create -f hack/manifests/jobset-operand.yaml + oc apply -f hack/manifests/jobset-operand.yaml🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 247 - 256, Replace non-idempotent "oc create -f hack/manifests/jobset-operator.yaml" and "oc create -f hack/manifests/jobset-operand.yaml" in the Makefile with "oc apply -f ..." so the install steps for the Jobset Operator and Jobset Instance can be retried safely; also update the similar pattern in the install-lws-operator target (where it uses oc create) to use oc apply for CI resilience. Ensure the rest of the waiting logic (timeout/until checks for deployments jobset-operator and jobset-controller-manager) remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Makefile`:
- Around line 247-256: Replace non-idempotent "oc create -f
hack/manifests/jobset-operator.yaml" and "oc create -f
hack/manifests/jobset-operand.yaml" in the Makefile with "oc apply -f ..." so
the install steps for the Jobset Operator and Jobset Instance can be retried
safely; also update the similar pattern in the install-lws-operator target
(where it uses oc create) to use oc apply for CI resilience. Ensure the rest of
the waiting logic (timeout/until checks for deployments jobset-operator and
jobset-controller-manager) remains unchanged.
MaysaMacedo
force-pushed
the
add-certmanager-upstream-tests
branch
from
8a6bf62 to
4143f60
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
upstream/kueue/patch/e2e.patch (2)
15-20:⚠️ Potential issue | 🟠 MajorCommented-out availability waits will cause flaky tests for JobSet/LeaderWorkerSet features.
WaitForJobSetAvailabilityandWaitForLeaderWorkerSetAvailabilityare removed with no replacement guards. Any test that carriesfeature:jobset,feature:tas,feature:trainjob, orfeature:leaderworkersetlabels will now proceed without confirming operator readiness, making those tests race-prone and non-deterministic. The PR objective is adding certmanager tests — the rationale for dropping these guards is unclear. If the CI target environment genuinely doesn't deploy those operators, the safer fix is to gate on environment variables or CI capabilities rather than silently removing readiness checks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@upstream/kueue/patch/e2e.patch` around lines 15 - 20, Restore the readiness waits for JobSet/LeaderWorkerSet instead of leaving them commented out: locate the ginkgo.Label(...).MatchesLabelFilter(labelFilter) checks and the calls to util.WaitForJobSetAvailability and util.WaitForLeaderWorkerSetAvailability and re-enable them, but guard them by environment/CI capability (e.g., skip when a SKIP_OPERATOR_CHECK or CI capability flag is set) so tests that carry feature:jobset, feature:tas, feature:trainjob, or feature:leaderworkerset labels wait for operator readiness unless explicitly skipped; ensure the logic uses the existing labelFilter and util.WaitFor... functions and document the new env flag in the test startup gating.
33-42:⚠️ Potential issue | 🟠 MajorPre-creation log is misleading and logs a stale, unlabeled namespace object; duplicate log on line 42 mixes logging styles.
Three problems in the rewrite of
CreateNamespaceFromObjectWithLog:
Wrong position & message (line 35):
ginkgo.GinkgoLogr.Info("Created namespace", "namespace", ns)fires before labels are set and beforek8sClient.Createis called. At that pointns.Nameis""(onlyGenerateNameis populated) and the label map hasn't been mutated yet, so the log captures incorrect state with a post-creation message.Duplicate log entries: Lines 35 and 42 both log a "Created namespace" event for the same operation. The post-creation log on line 42 is the only one that carries accurate state.
Mixed logging style (line 42):
ginkgo.GinkgoLogr.Info(fmt.Sprintf("Created namespace: %s", ns.Name))passes a pre-formatted string where thelogrconvention is structured key-value pairs, inconsistent with line 35.🐛 Proposed fix
func CreateNamespaceFromObjectWithLog(ctx context.Context, k8sClient client.Client, ns *corev1.Namespace) *corev1.Namespace { - ginkgo.GinkgoLogr.Info("Created namespace", "namespace", ns) - // Add label to the namespace to mark it as managed by Kueue. if ns.Labels == nil { ns.Labels = make(map[string]string) } ns.Labels["kueue.openshift.io/managed"] = "true" gomega.Expect(k8sClient.Create(ctx, ns)).To(gomega.Succeed()) - ginkgo.GinkgoLogr.Info(fmt.Sprintf("Created namespace: %s", ns.Name)) + ginkgo.GinkgoLogr.Info("Created namespace", "namespace", ns.Name) return ns }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@upstream/kueue/patch/e2e.patch` around lines 33 - 42, The pre-creation Info call is logging stale state and duplicates the later log; remove the early ginkgo.GinkgoLogr.Info("Created namespace", "namespace", ns), ensure you populate ns.Labels (make map if nil) and set ns.Labels["kueue.openshift.io/managed"]="true" before calling k8sClient.Create(ctx, ns), keep the gomega.Expect(k8sClient.Create(ctx, ns)).To(gomega.Succeed()), and replace the trailing fmt.Sprintf log with a single structured log using ginkgo.GinkgoLogr.Info("Created namespace", "namespace", ns.Name) immediately after the successful create so only one accurate, structured message is emitted.
🧹 Nitpick comments (1)
upstream/kueue/patch/e2e.patch (1)
53-60: Label injection is correct but inconsistent with the centralised path inCreateNamespaceFromObjectWithLog.The explicit
"kueue.openshift.io/managed": "true"label inBeforeEachis necessary here because the test usesutil.MustCreatedirectly rather thanutil.CreateNamespaceFromObjectWithLog, which now injects this label automatically. The change is functionally correct as-is.Optionally, switching to
util.CreateNamespaceFromObjectWithLogwould centralise label injection and remove the need to duplicate it here:♻️ Optional: replace MustCreate with the labelling utility
- ns = &corev1.Namespace{ - ObjectMeta: metav1.ObjectMeta{ - GenerateName: "e2e-cert-manager-", - Labels: map[string]string{"kueue.openshift.io/managed": "true"}, - }, - } - util.MustCreate(ctx, k8sClient, ns) + ns = util.CreateNamespaceFromObjectWithLog(ctx, k8sClient, &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{GenerateName: "e2e-cert-manager-"}, + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@upstream/kueue/patch/e2e.patch` around lines 53 - 60, The namespace creation in the BeforeEach currently adds the label "kueue.openshift.io/managed": "true" inline when constructing ns and calls util.MustCreate, causing duplication with the centralized labelling behavior in util.CreateNamespaceFromObjectWithLog; fix by replacing the util.MustCreate(ctx, k8sClient, ns) call with util.CreateNamespaceFromObjectWithLog (or the equivalent util helper) so the label injection is centralized, and remove the explicit Labels map from the ns ObjectMeta (or keep only if util helper isn't used) — target symbols: ns, util.MustCreate, util.CreateNamespaceFromObjectWithLog, and the label key "kueue.openshift.io/managed".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@upstream/kueue/patch/e2e.patch`:
- Around line 15-20: Restore the readiness waits for JobSet/LeaderWorkerSet
instead of leaving them commented out: locate the
ginkgo.Label(...).MatchesLabelFilter(labelFilter) checks and the calls to
util.WaitForJobSetAvailability and util.WaitForLeaderWorkerSetAvailability and
re-enable them, but guard them by environment/CI capability (e.g., skip when a
SKIP_OPERATOR_CHECK or CI capability flag is set) so tests that carry
feature:jobset, feature:tas, feature:trainjob, or feature:leaderworkerset labels
wait for operator readiness unless explicitly skipped; ensure the logic uses the
existing labelFilter and util.WaitFor... functions and document the new env flag
in the test startup gating.
- Around line 33-42: The pre-creation Info call is logging stale state and
duplicates the later log; remove the early ginkgo.GinkgoLogr.Info("Created
namespace", "namespace", ns), ensure you populate ns.Labels (make map if nil)
and set ns.Labels["kueue.openshift.io/managed"]="true" before calling
k8sClient.Create(ctx, ns), keep the gomega.Expect(k8sClient.Create(ctx,
ns)).To(gomega.Succeed()), and replace the trailing fmt.Sprintf log with a
single structured log using ginkgo.GinkgoLogr.Info("Created namespace",
"namespace", ns.Name) immediately after the successful create so only one
accurate, structured message is emitted.
---
Nitpick comments:
In `@upstream/kueue/patch/e2e.patch`:
- Around line 53-60: The namespace creation in the BeforeEach currently adds the
label "kueue.openshift.io/managed": "true" inline when constructing ns and calls
util.MustCreate, causing duplication with the centralized labelling behavior in
util.CreateNamespaceFromObjectWithLog; fix by replacing the util.MustCreate(ctx,
k8sClient, ns) call with util.CreateNamespaceFromObjectWithLog (or the
equivalent util helper) so the label injection is centralized, and remove the
explicit Labels map from the ns ObjectMeta (or keep only if util helper isn't
used) — target symbols: ns, util.MustCreate,
util.CreateNamespaceFromObjectWithLog, and the label key
"kueue.openshift.io/managed".
4143f60 to
3820c39
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
upstream/kueue/patch/e2e.patch (1)
32-44:⚠️ Potential issue | 🟡 MinorTwo log issues in
CreateNamespaceFromObjectWithLog: misleading pre-creation message and unstructured post-creation call.
Line 35 logs
"Created namespace"before the namespace is created (before line 41'sk8sClient.Create). The message is factually incorrect at that point. Additionally, it passes the entirensstruct as the value rather than a field, which serialises the full Kubernetes object into the log.Line 42 uses
fmt.Sprintfinside a structuredlogr-based logger, discarding the key-value contract of the logger and producing an unstructured message — inconsistent with the original code and thelogrAPI.The cleanest fix is to remove the premature log and restore the original structured form after creation:
🔧 Proposed fix
func CreateNamespaceFromObjectWithLog(ctx context.Context, k8sClient client.Client, ns *corev1.Namespace) *corev1.Namespace { - ginkgo.GinkgoLogr.Info("Created namespace", "namespace", ns) - // Add label to the namespace to mark it as managed by Kueue. + // Add label to the namespace to mark it as managed by Kueue. if ns.Labels == nil { ns.Labels = make(map[string]string) } ns.Labels["kueue.openshift.io/managed"] = "true" gomega.Expect(k8sClient.Create(ctx, ns)).To(gomega.Succeed()) - ginkgo.GinkgoLogr.Info(fmt.Sprintf("Created namespace: %s", ns.Name)) + ginkgo.GinkgoLogr.Info("Created namespace", "namespace", ns.Name) return ns }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@upstream/kueue/patch/e2e.patch` around lines 32 - 44, In CreateNamespaceFromObjectWithLog, remove the premature ginkgo.GinkgoLogr.Info call that logs "Created namespace" before k8sClient.Create executes and avoid passing the whole ns object; instead keep the label-addition and the Expect(k8sClient.Create(ctx, ns)).To(gomega.Succeed()) call, then log the creation using the structured logr API (ginkgo.GinkgoLogr.Info) with a key "namespace" and value ns.Name (not fmt.Sprintf and not the entire ns struct) so the message is factual and respects the logger's key/value contract.
🧹 Nitpick comments (1)
upstream/kueue/patch/e2e.patch (1)
9-20: Consider removing the commented-out availability checks rather than retaining them.Leaving commented-out code in a patch-file creates maintenance confusion — it's unclear whether this is intentional, temporary, or accidentally left in. If JobSet/LeaderWorkerSet availability waits are not needed for OpenShift, delete the blocks outright.
🧹 Proposed cleanup
waitForAvailableStart := time.Now() util.WaitForKueueAvailability(ctx, k8sClient) labelFilter := ginkgo.GinkgoLabelFilter() - // if ginkgo.Label("feature:jobset", "feature:tas", "feature:trainjob").MatchesLabelFilter(labelFilter) { - // util.WaitForJobSetAvailability(ctx, k8sClient) - // } - // if ginkgo.Label("feature:leaderworkerset").MatchesLabelFilter(labelFilter) { - // util.WaitForLeaderWorkerSetAvailability(ctx, k8sClient) - // } if ginkgo.Label("feature:appwrapper").MatchesLabelFilter(labelFilter) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@upstream/kueue/patch/e2e.patch` around lines 9 - 20, The patch leaves two commented-out blocks that call ginkgo.Label(...).MatchesLabelFilter(labelFilter) and then util.WaitForJobSetAvailability(ctx, k8sClient) / util.WaitForLeaderWorkerSetAvailability(ctx, k8sClient); remove these commented lines entirely (delete both commented ginkgo.Label checks and the commented util.WaitFor... calls) so the patch does not retain dead/commented code and the intent is clear.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@upstream/kueue/patch/e2e.patch`:
- Around line 32-44: In CreateNamespaceFromObjectWithLog, remove the premature
ginkgo.GinkgoLogr.Info call that logs "Created namespace" before
k8sClient.Create executes and avoid passing the whole ns object; instead keep
the label-addition and the Expect(k8sClient.Create(ctx,
ns)).To(gomega.Succeed()) call, then log the creation using the structured logr
API (ginkgo.GinkgoLogr.Info) with a key "namespace" and value ns.Name (not
fmt.Sprintf and not the entire ns struct) so the message is factual and respects
the logger's key/value contract.
---
Nitpick comments:
In `@upstream/kueue/patch/e2e.patch`:
- Around line 9-20: The patch leaves two commented-out blocks that call
ginkgo.Label(...).MatchesLabelFilter(labelFilter) and then
util.WaitForJobSetAvailability(ctx, k8sClient) /
util.WaitForLeaderWorkerSetAvailability(ctx, k8sClient); remove these commented
lines entirely (delete both commented ginkgo.Label checks and the commented
util.WaitFor... calls) so the patch does not retain dead/commented code and the
intent is clear.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
upstream/kueue/patch/e2e.patch (1)
35-35:⚠️ Potential issue | 🟡 MinorPre-creation log fires before the namespace exists and before
GenerateNameis resolved.The log message
"Created namespace"on line 35 is emitted beforek8sClient.Createon line 41. When the caller passes aGenerateName-based namespace (as incertmanager_test.go),ns.Nameis empty at this point — the server only populates it after the API call. If theCreatelater fails, a misleading"Created namespace"entry still appears in the test log. This pre-creation line should either be removed or changed to"Creating namespace"to accurately describe the intent.🛠️ Proposed fix
func CreateNamespaceFromObjectWithLog(ctx context.Context, k8sClient client.Client, ns *corev1.Namespace) *corev1.Namespace { - ginkgo.GinkgoLogr.Info("Created namespace", "namespace", ns) - // Add label to the namespace to mark it as managed by Kueue. + // Add label to the namespace to mark it as managed by Kueue. if ns.Labels == nil { ns.Labels = make(map[string]string) } ns.Labels["kueue.openshift.io/managed"] = "true" gomega.Expect(k8sClient.Create(ctx, ns)).To(gomega.Succeed()) - ginkgo.GinkgoLogr.Info(fmt.Sprintf("Created namespace: %s", ns.Name)) + ginkgo.GinkgoLogr.Info("Created namespace", "name", ns.Name) return ns }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@upstream/kueue/patch/e2e.patch` at line 35, The log currently uses ginkgo.GinkgoLogr.Info("Created namespace", "namespace", ns) before the call to k8sClient.Create which can be misleading for GenerateName namespaces; change that pre-create log to ginkgo.GinkgoLogr.Info("Creating namespace", "namespace", ns) so it accurately reflects intent, and after k8sClient.Create succeeds emit a separate ginkgo.GinkgoLogr.Info("Created namespace", "namespace", ns.Name) (or include ns.Name) to record the actual created name; update the logging around k8sClient.Create to reflect this two-step approach.
🧹 Nitpick comments (2)
upstream/kueue/patch/e2e.patch (2)
15-20: Consider removing commented-out blocks rather than leaving them in-place.Commenting out the
WaitForJobSetAvailability/WaitForLeaderWorkerSetAvailabilitycalls leaves dead code in the patch that will accumulate over time. If these waits genuinely cannot run in the OpenShift CI environment, removing the lines (or guarding them with an env-var/build tag) communicates intent more clearly than comments.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@upstream/kueue/patch/e2e.patch` around lines 15 - 20, Remove the dead commented-out wait blocks: delete the commented ginkgo.Label(...) branches that call util.WaitForJobSetAvailability and util.WaitForLeaderWorkerSetAvailability (or, if the waits must be conditionally disabled in CI, replace the comments with an explicit guard using an environment variable or build tag around the actual calls such as checking an env var like RUN_JOBSET_WAITS before calling util.WaitForJobSetAvailability/WaitForLeaderWorkerSetAvailability). This keeps intent explicit and avoids accumulating commented-out code.
42-42: Use structured key-value args instead offmt.SprintfwithGinkgoLogr.Info.
logr.Logger.Infoaccepts a message string plus variadic key-value pairs; wrapping the entire message infmt.Sprintfdiscards the structured-logging benefit and is non-idiomatic.♻️ Proposed fix
- ginkgo.GinkgoLogr.Info(fmt.Sprintf("Created namespace: %s", ns.Name)) + ginkgo.GinkgoLogr.Info("Created namespace", "name", ns.Name)If this is the only remaining
fmtusage after the fix, thefmtimport can be removed as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@upstream/kueue/patch/e2e.patch` at line 42, Replace the fmt.Sprintf usage when logging namespace creation with structured key/value args: call ginkgo.GinkgoLogr.Info with a short message like "Created namespace" and pass "namespace" and ns.Name as the key/value pair (i.e., ginkgo.GinkgoLogr.Info("Created namespace", "namespace", ns.Name)); then remove the fmt import if it becomes unused. This change targets the ginkgo.GinkgoLogr.Info call that currently wraps fmt.Sprintf and preserves structured logging semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@upstream/kueue/patch/e2e.patch`:
- Line 35: The log currently uses ginkgo.GinkgoLogr.Info("Created namespace",
"namespace", ns) before the call to k8sClient.Create which can be misleading for
GenerateName namespaces; change that pre-create log to
ginkgo.GinkgoLogr.Info("Creating namespace", "namespace", ns) so it accurately
reflects intent, and after k8sClient.Create succeeds emit a separate
ginkgo.GinkgoLogr.Info("Created namespace", "namespace", ns.Name) (or include
ns.Name) to record the actual created name; update the logging around
k8sClient.Create to reflect this two-step approach.
---
Nitpick comments:
In `@upstream/kueue/patch/e2e.patch`:
- Around line 15-20: Remove the dead commented-out wait blocks: delete the
commented ginkgo.Label(...) branches that call util.WaitForJobSetAvailability
and util.WaitForLeaderWorkerSetAvailability (or, if the waits must be
conditionally disabled in CI, replace the comments with an explicit guard using
an environment variable or build tag around the actual calls such as checking an
env var like RUN_JOBSET_WAITS before calling
util.WaitForJobSetAvailability/WaitForLeaderWorkerSetAvailability). This keeps
intent explicit and avoids accumulating commented-out code.
- Line 42: Replace the fmt.Sprintf usage when logging namespace creation with
structured key/value args: call ginkgo.GinkgoLogr.Info with a short message like
"Created namespace" and pass "namespace" and ns.Name as the key/value pair
(i.e., ginkgo.GinkgoLogr.Info("Created namespace", "namespace", ns.Name)); then
remove the fmt import if it becomes unused. This change targets the
ginkgo.GinkgoLogr.Info call that currently wraps fmt.Sprintf and preserves
structured logging semantics.
|
/retest |
MaysaMacedo
force-pushed
the
add-certmanager-upstream-tests
branch
from
6bc544e to
b3099d0
Compare
|
@anahas-redhat Hey, this PR helps with running the certmanager suite, we can see that this test scenario is passing: |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
upstream/kueue/patch/e2e.patch (1)
32-44:⚠️ Potential issue | 🟡 MinorMisleading pre-creation log and non-idiomatic
fmt.Sprintfin logr context.Three interrelated issues in
CreateNamespaceFromObjectWithLog:
Log before create:
ginkgo.GinkgoLogr.Info("Created namespace", …)(past tense) fires before the labels are mutated and beforek8sClient.Createis called. The loggednsobject therefore lacks thekueue.openshift.io/managedlabel, giving a misleading picture in test logs.Verbose structured value: Passing the full
*corev1.Namespacepointer as the logr value produces the entire object dump. The originalns.Namewas more appropriate.Non-idiomatic fmt.Sprintf in logr:
ginkgo.GinkgoLogr.Info(fmt.Sprintf("Created namespace: %s", ns.Name))wraps a string message withfmt.Sprintfinstead of using structured key-value pairs, mixing printf-style and structured logging.♻️ Proposed fix
func CreateNamespaceFromObjectWithLog(ctx context.Context, k8sClient client.Client, ns *corev1.Namespace) *corev1.Namespace { - ginkgo.GinkgoLogr.Info("Created namespace", "namespace", ns) - // Add label to the namespace to mark it as managed by Kueue. + // Add label to the namespace to mark it as managed by Kueue. if ns.Labels == nil { ns.Labels = make(map[string]string) } ns.Labels["kueue.openshift.io/managed"] = "true" gomega.Expect(k8sClient.Create(ctx, ns)).To(gomega.Succeed()) - ginkgo.GinkgoLogr.Info(fmt.Sprintf("Created namespace: %s", ns.Name)) + ginkgo.GinkgoLogr.Info("Created namespace", "namespace", ns.Name) return ns }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@upstream/kueue/patch/e2e.patch` around lines 32 - 44, CreateNamespaceFromObjectWithLog currently logs "Created namespace" before mutating labels and creating the Namespace, logs the entire Namespace pointer, and uses fmt.Sprintf instead of structured logr args; fix by first ensuring ns.Labels is initialized and adding ns.Labels["kueue.openshift.io/managed"]="true", then call k8sClient.Create(ctx, ns) and assert success (e.g., gomega.Expect(...).To(gomega.Succeed())), and only after successful creation call ginkgo.GinkgoLogr.Info("Created namespace", "namespace", ns.Name) using the structured key "namespace" (remove fmt.Sprintf and avoid passing the full *corev1.Namespace).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Makefile`:
- Around line 234-243: The e2e pipeline currently stops if
./upstream/kueue/e2e-test-ocp.sh exits non-zero so the certmanager block (the
KUEUE_NAMESPACE=... $(GINKGO) $(GINKGO_ARGS) ...
./upstream/kueue/src/test/e2e/certmanager/...) never runs; change the Make
recipe to tolerate the first script failing by capturing its exit code and
continuing (e.g., run ./upstream/kueue/e2e-test-ocp.sh and save $? into a
variable or append a tolerant operator so execution proceeds), then
unconditionally run the certmanager Ginkgo invocation, and finally, if desired,
exit the recipe with the original saved exit code to preserve overall failure
status.
---
Outside diff comments:
In `@upstream/kueue/patch/e2e.patch`:
- Around line 32-44: CreateNamespaceFromObjectWithLog currently logs "Created
namespace" before mutating labels and creating the Namespace, logs the entire
Namespace pointer, and uses fmt.Sprintf instead of structured logr args; fix by
first ensuring ns.Labels is initialized and adding
ns.Labels["kueue.openshift.io/managed"]="true", then call k8sClient.Create(ctx,
ns) and assert success (e.g., gomega.Expect(...).To(gomega.Succeed())), and only
after successful creation call ginkgo.GinkgoLogr.Info("Created namespace",
"namespace", ns.Name) using the structured key "namespace" (remove fmt.Sprintf
and avoid passing the full *corev1.Namespace).
@MaysaMacedo thanks for executing the tests. I'm closing the related jira bug. If this works, we may be able to add other tests (outside singlecluster folder) to Kueue CI. |
MaysaMacedo
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
MaysaMacedo
force-pushed
the
add-certmanager-upstream-tests
branch
2 times, most recently
from
4348bbf to
ad53e11
Compare
MaysaMacedo
force-pushed
the
add-certmanager-upstream-tests
branch
from
ad53e11 to
b09cda1
Compare
MaysaMacedo
force-pushed
the
add-certmanager-upstream-tests
branch
from
b09cda1 to
baf1f48
Compare
MaysaMacedo
force-pushed
the
add-certmanager-upstream-tests
branch
2 times, most recently
from
bb42d3b to
55a4d6c
Compare
Assisted-by: Cursor AI
MaysaMacedo
force-pushed
the
add-certmanager-upstream-tests
branch
2 times, most recently
from
e9ef84a to
de39c3c
Compare
Generated by: Claude
MaysaMacedo
force-pushed
the
add-certmanager-upstream-tests
branch
from
de39c3c to
026d336
Compare
|
@MaysaMacedo: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
/lgtm |
4 participants
Assisted-by: Cursor AI