USHIFT-6743: migrate 9 api-server tests from openshift-tests-private to RF#6625
Open
agullon wants to merge 10 commits intoopenshift:mainfrom
Open
USHIFT-6743: migrate 9 api-server tests from openshift-tests-private to RF#6625agullon wants to merge 10 commits intoopenshift:mainfrom
agullon wants to merge 10 commits intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@agullon: This pull request references USHIFT-6743 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
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 multiple Robot Framework test suites and a kustomize test resource to validate MicroShift configuration and behavior (API server readiness, audit logging, data directory, drop-in configs, kustomize sources, logging, show-config). Implements a shared keyword ChangesShared Test Infrastructure
Kustomize Test Resource
Configuration Test Suites
Sequence Diagram(s)sequenceDiagram
rect rgba(200,200,255,0.5)
participant Tester as Test Runner
participant Host as Remote Host (ssh)
participant Service as systemd / MicroShift
participant API as kube-apiserver
end
Tester->>Host: systemctl stop microshift
Tester->>Host: systemctl start microshift --no-block
Host->>Service: start MicroShift (async)
loop poll until ready or timeout
Tester->>API: curl --header X-MicroShift-Reject: true /readyz
alt during startup
API-->>Tester: HTTP 429 (rejected)
else once initialized
API-->>Tester: HTTP 200/401/403 (ready-like)
end
end
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: agullon 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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/suites/configuration/apiserver-readiness.robot`:
- Around line 30-33: Teardown currently calls the keywords in the wrong order:
"Wait For MicroShift" runs before "Setup Kubeconfig", which can cause spurious
failures if kubeconfig changes; modify the teardown sequence so that Setup
Kubeconfig is executed before Wait For MicroShift by swapping the two keyword
calls in the teardown block (reference keywords: Setup Kubeconfig, Wait For
MicroShift).
In `@test/suites/configuration/audit-log.robot`:
- Around line 175-176: The grep invocation in the Robot test (where you build
the shell command that uses ${AUDIT_LOG} and "${resource_name}" and then RETURN
${stdout.strip()}) is appending an extra "0" via `|| echo 0`, producing "0\n0"
and breaking integer assertions; replace `grep -c ... || echo 0` with either
just `grep -c ...` (grep will print 0 on no matches) or `grep -c ... || true` to
preserve a single "0" output and suppress the non-zero exit code so RETURN
${stdout.strip()} yields a single numeric string usable by Should Be Equal As
Integers.
In `@test/suites/configuration/drop-in-config.robot`:
- Around line 78-80: The test currently only asserts a non-zero exit code for
the oc get command, which is too broad; after running Run Process (which merges
stderr into stdout via stderr=STDOUT) check that the output specifically
indicates a NotFound error. Replace or add to the numeric check by asserting the
combined output variable ${result.stdout} contains the NotFound message (e.g.,
use the Robot keyword Should Contain ${result.stdout} NotFound or a regex like
"not found") while still preserving the non-zero rc check if desired; reference
the existing Run Process call and the ${result} variable to locate where to add
the Should Contain assertion instead of relying solely on Should Not Be Equal As
Integers ${result.rc} 0.
In `@test/suites/configuration/kustomize-sources.robot`:
- Around line 192-194: The test currently treats any non-zero return code from
the oc get configmap command as success; tighten it by also asserting the
command output explicitly indicates a not-found condition: after the Run Process
call that sets ${result}, keep the existing check on ${result.rc} but add an
assertion that ${result.stdout} (or ${result.stderr} if you prefer) contains a
not-found marker such as "NotFound", "not found", or "Error from server
(NotFound)" for the specific ${CONFIGMAP_NAME} in ${namespace}; use Robot
keywords like Should Contain to validate ${result.stdout} (and/or Should Contain
to check ${result.stderr}) so the spec only passes when the configmap is
actually absent.
In `@test/suites/configuration/show-config.robot`:
- Around line 77-80: The teardown currently runs "Command Should Work rm -rf
${HOME_CONFIG_DIR}" which deletes the entire ${HOME_CONFIG_DIR}; change the
teardown to remove only the specific file created by this test (e.g.,
${HOME_CONFIG_DIR}/config.yaml) instead of recursively deleting
${HOME_CONFIG_DIR}, keeping the other teardown steps (Run Keywords, Command
Should Work, AND, Remove Drop In MicroShift Config 10-memlimit) intact; update
the "Command Should Work" invocation so it targets the single file path rather
than using rm -rf on ${HOME_CONFIG_DIR}.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 97f1c51e-3ebd-409d-bdb2-698373ecbfbc
📒 Files selected for processing (9)
test/resources/microshift-process.resourcetest/suites/configuration/apiserver-readiness.robottest/suites/configuration/audit-log.robottest/suites/configuration/data-dir.robottest/suites/configuration/drop-in-config.robottest/suites/configuration/kustomize-sources.robottest/suites/configuration/logging.robottest/suites/configuration/show-config.robottest/suites/osconfig/lifecycle.robot
💤 Files with no reviewable changes (1)
- test/suites/osconfig/lifecycle.robot
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
test/suites/configuration/audit-log.robot (1)
191-193:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
grep -c ... || echo 0can return0\n0and break numeric assertionsAt Line 192, Line 199, Line 206, and Line 213, the fallback appends an extra zero on no-match paths, so
${stdout.strip()}may not be a valid integer.Proposed minimal fix
- ... grep -c '"${resource_name}"' ${AUDIT_LOG} || echo 0 + ... grep -c '"${resource_name}"' ${AUDIT_LOG} || true ... - ... grep '"${resource_name}"' ${AUDIT_LOG} | grep -c '"requestObject"\\|"responseObject"' || echo 0 + ... grep '"${resource_name}"' ${AUDIT_LOG} | grep -c '"requestObject"\\|"responseObject"' || true ... - ... grep '"${resource_name}"' ${AUDIT_LOG} | grep -E '"verb":"(create|update|patch|delete)"' | grep -c '"requestObject"' || echo 0 + ... grep '"${resource_name}"' ${AUDIT_LOG} | grep -E '"verb":"(create|update|patch|delete)"' | grep -c '"requestObject"' || true ... - ... grep '"${resource_name}"' ${AUDIT_LOG} | grep -E '"verb":"(get|list|watch)"' | grep -c '"responseObject"' || echo 0 + ... grep '"${resource_name}"' ${AUDIT_LOG} | grep -E '"verb":"(get|list|watch)"' | grep -c '"responseObject"' || trueAlso applies to: 198-200, 205-207, 212-214
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/suites/configuration/audit-log.robot` around lines 191 - 193, The grep fallback can emit multiple lines (e.g. "0\n0") and break numeric assertions when the test reads RETURN ${stdout.strip()}; update the shell expression used with the Command Should Work call that runs grep '"${resource_name}"' ${AUDIT_LOG} so it always yields a single integer string — for example, pipe the grep output to tail -n1 (or use an equivalent awk/printf to print only the last/first line) so ${stdout.strip()} is a valid integer; apply this change to every occurrence of the grep '"${resource_name}"' ${AUDIT_LOG} pattern in the file referenced by the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/resources/kustomize-test.resource`:
- Around line 27-30: The Robot keyword "Remove Manifest Directory" currently
runs "rm -rf ${manifest_dir}" unsafely; change it to validate and normalize
${manifest_dir} before deletion: ensure ${manifest_dir} is not empty, not "/",
and is under an allowed base (e.g., the repo/test workspace or a known temp dir)
by resolving to an absolute path and comparing prefixes, only then run the
removal; also explicitly fail the keyword with a clear error if validation fails
or the path is outside the allowed directory to prevent accidental host-wide
deletions.
In `@test/suites/configuration/apiserver-readiness.robot`:
- Around line 49-60: The readiness poll loop currently sleeps for 0.5s between
attempts, which can miss short 429 windows; in the FOR ${i} IN RANGE loop that
runs the curl against ${APIS_ENDPOINT} with header ${READINESS_HEADER}, reduce
the Sleep interval (e.g., to 0.1s or 0.2s) to catch transient 429 responses and,
if you want to preserve the same overall timeout, proportionally increase the
iteration count (FOR ${i} IN RANGE) so total wait time remains acceptable.
---
Duplicate comments:
In `@test/suites/configuration/audit-log.robot`:
- Around line 191-193: The grep fallback can emit multiple lines (e.g. "0\n0")
and break numeric assertions when the test reads RETURN ${stdout.strip()};
update the shell expression used with the Command Should Work call that runs
grep '"${resource_name}"' ${AUDIT_LOG} so it always yields a single integer
string — for example, pipe the grep output to tail -n1 (or use an equivalent
awk/printf to print only the last/first line) so ${stdout.strip()} is a valid
integer; apply this change to every occurrence of the grep '"${resource_name}"'
${AUDIT_LOG} pattern in the file referenced by the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 21e46a09-2beb-43cd-85bb-f365a4f92b2e
📒 Files selected for processing (5)
test/resources/kustomize-test.resourcetest/suites/configuration/apiserver-readiness.robottest/suites/configuration/audit-log.robottest/suites/configuration/drop-in-config.robottest/suites/configuration/kustomize-sources.robot
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
test/suites/configuration/drop-in-config.robot (1)
60-64: ⚡ Quick winUse failure-tolerant teardown sequencing.
Line 60 uses
Run Keywords ... AND ...; a single cleanup failure can block later cleanup/restart and contaminate subsequent tests. ConsiderRun Keyword And Continue On Failureper step (or a shared resilient cleanup keyword).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/suites/configuration/drop-in-config.robot` around lines 60 - 64, The teardown currently chains cleanup steps using "Run Keywords" so a single failure can abort subsequent cleanup; update the teardown to run each step with failure-tolerant execution (e.g., replace the chained "Run Keywords" invocation with individual "Run Keyword And Continue On Failure" calls for "Remove Drop In MicroShift Config", "Remove Manifest Directory", "Oc Delete" and "Restart MicroShift", or implement a single resilient cleanup keyword that internally calls those functions with continue-on-failure behavior) to ensure all cleanup steps execute even if one fails.test/suites/configuration/kustomize-sources.robot (1)
88-95: ⚡ Quick winMake teardown cleanup non-short-circuiting.
Line 88 uses
Run Keywords ... AND ...; if one cleanup step fails, later steps (including restart) may be skipped, leaving state behind for following tests. Prefer per-stepRun Keyword And Continue On Failure(or a helper keyword implementing that behavior).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/suites/configuration/kustomize-sources.robot` around lines 88 - 95, The teardown currently uses a single "Run Keywords" chain which short-circuits on the first failure; replace each chained step (the "Run Keywords" invocation involving Remove Drop In MicroShift Config, Remove Manifest Directory for ${MANIFEST_DIR_1} and ${MANIFEST_DIR_2}, Oc Delete namespace ksrc-multi-ns-1/ksrc-multi-ns-2, and Restart MicroShift) with individual calls to "Run Keyword And Continue On Failure" (or a small helper keyword that wraps each of those: Remove Drop In MicroShift Config, Remove Manifest Directory, Oc Delete, Restart MicroShift) so every cleanup action runs even if prior ones fail.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/suites/configuration/drop-in-config.robot`:
- Around line 74-79: The negative assertion for dropin-ns-a2 is currently
checked immediately after "Restart MicroShift" which is flaky; wrap the
"ConfigMap Should Not Exist dropin-ns-a2" call in a "Wait Until Keyword
Succeeds" block (use the same retry parameters as the preceding check, e.g.,
"10x 10s") so the test retries the negative assertion until the API is fully
up; update the sequence that contains "Restart MicroShift", "Oc Get configmap
dropin-ns-b test-configmap", and the "ConfigMap Should Not Exist" check to
use this wait wrapper around the latter.
In `@test/suites/configuration/kustomize-sources.robot`:
- Around line 57-60: The negative ConfigMap existence assertion runs immediately
after restart and can fail due to transient API startup; wrap the "ConfigMap
Should Not Exist ksrc-empty-ns" call in Robot's "Wait Until Keyword Succeeds"
to poll until the API is stable (e.g., retry every 5s for up to 60s). Replace
the direct call to ConfigMap Should Not Exist in the test with a Wait Until
Keyword Succeeds that invokes ConfigMap Should Not Exist and provide sensible
retry interval and timeout to ensure a stable post-start state.
---
Nitpick comments:
In `@test/suites/configuration/drop-in-config.robot`:
- Around line 60-64: The teardown currently chains cleanup steps using "Run
Keywords" so a single failure can abort subsequent cleanup; update the teardown
to run each step with failure-tolerant execution (e.g., replace the chained "Run
Keywords" invocation with individual "Run Keyword And Continue On Failure" calls
for "Remove Drop In MicroShift Config", "Remove Manifest Directory", "Oc Delete"
and "Restart MicroShift", or implement a single resilient cleanup keyword that
internally calls those functions with continue-on-failure behavior) to ensure
all cleanup steps execute even if one fails.
In `@test/suites/configuration/kustomize-sources.robot`:
- Around line 88-95: The teardown currently uses a single "Run Keywords" chain
which short-circuits on the first failure; replace each chained step (the "Run
Keywords" invocation involving Remove Drop In MicroShift Config, Remove Manifest
Directory for ${MANIFEST_DIR_1} and ${MANIFEST_DIR_2}, Oc Delete namespace
ksrc-multi-ns-1/ksrc-multi-ns-2, and Restart MicroShift) with individual calls
to "Run Keyword And Continue On Failure" (or a small helper keyword that wraps
each of those: Remove Drop In MicroShift Config, Remove Manifest Directory, Oc
Delete, Restart MicroShift) so every cleanup action runs even if prior ones
fail.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 9e48c90a-b00c-4e13-a5cc-b0ac1c3624cf
📒 Files selected for processing (3)
test/suites/configuration/audit-log.robottest/suites/configuration/drop-in-config.robottest/suites/configuration/kustomize-sources.robot
agullon
added a commit
to agullon/microshift
that referenced
this pull request
May 6, 2026
Auto-applied: - audit-log.robot: replace `|| echo 0` with `|| true` to fix 0\n0 output Accepted after review: - kustomize-test.resource: assert NotFound in ConfigMap Should Not Exist - kustomize-test.resource: guard rm -rf with path validation - apiserver-readiness.robot: reduce polling interval to 0.2s (300 iterations) - show-config.robot: narrow teardown to rm -f config.yaml only Co-Authored-By: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> pre-commit.check-secrets: ENABLED
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
test/resources/kustomize-test.resource (1)
30-32:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDeletion guard is still incomplete; normalize and block base-root deletes.
Line 31 checks raw prefix only. A traversal-style path can bypass intent, and Line 32 still allows deleting
/etc/microshift/itself.Suggested patch
Remove Manifest Directory [Documentation] Completely remove a manifest directory under /etc/microshift [Arguments] ${manifest_dir} Should Not Be Empty ${manifest_dir} - Should Start With ${manifest_dir} /etc/microshift/ - Command Should Work rm -rf ${manifest_dir} + ${normalized}= Evaluate os.path.abspath(os.path.normpath(r'''${manifest_dir}''')) os + Should Start With ${normalized} /etc/microshift/ + Should Not Be Equal ${normalized} /etc/microshift + Should Not Be Equal ${normalized} / + Command Should Work rm -rf -- '${normalized}'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/resources/kustomize-test.resource` around lines 30 - 32, The test's deletion guard around ${manifest_dir} is insufficient because it only checks raw prefix and allows traversal or deletion of the base root; update the test to normalize the path (resolve symlinks and collapse .. components) before checks and assert that the normalized path is a strict child of the intended base root (not equal to it) and not outside it; replace the "Should Start With ${manifest_dir} /etc/microshift/" check with a normalized containment assertion and add a check that the normalized ${manifest_dir} != /etc/microshift/ to prevent deleting the base-root, then only run "Command Should Work rm -rf ${manifest_dir}" after these validations pass.
🧹 Nitpick comments (2)
test/resources/kustomize-test.resource (1)
12-12: ⚡ Quick winQuote and terminate
mkdirargs to harden path handling.Line 12 should pass
${manifest_dir}as a single shell arg (--+ quotes) to avoid edge-case breakage from whitespace/special chars.Suggested patch
- Command Should Work mkdir -p ${manifest_dir} + Command Should Work mkdir -p -- '${manifest_dir}'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/resources/kustomize-test.resource` at line 12, Replace the unquoted mkdir invocation in the "Command Should Work" test so ${manifest_dir} is passed as a single shell argument and options are terminated; specifically modify the test line that currently uses "mkdir -p ${manifest_dir}" (the "Command Should Work" step) to call mkdir with a "--" terminator and quote the ${manifest_dir} variable so paths with spaces, special chars or a leading dash are handled safely.test/suites/configuration/audit-log.robot (1)
87-90: ⚡ Quick winAvoid fixed ConfigMap names when grepping a persistent audit log.
Lines 87/103/124/144 use static names, but Lines 188-214 grep the entire
${AUDIT_LOG}. Old entries from previous runs can be matched, making assertions flaky (especially zero-count checks).Suggested refactor
+ ${run_id}= Evaluate __import__("uuid").uuid4().hex[:8] - ${cm_name}= Set Variable audit-none-cm + ${cm_name}= Set Variable audit-none-cm-${run_id}Apply the same pattern to
audit-default-cm,audit-write-cm, andaudit-all-cm.Also applies to: 103-107, 124-128, 144-148, 188-214
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/suites/configuration/audit-log.robot` around lines 87 - 90, The tests use fixed ConfigMap names (e.g., audit-none-cm) when calling Oc Create and then call the keyword Grep Audit Log Count against ${AUDIT_LOG}, which can match stale entries from prior runs and make zero-count checks flaky; update the test to generate and use unique ConfigMap names for each create (e.g., append a run-specific suffix/timestamp or ${TEST_RUN_ID}) and pass that generated name into the Grep Audit Log Count call (references: the variable ${cm_name}, the keyword Grep Audit Log Count, and the audit ConfigMap names audit-none-cm, audit-default-cm, audit-write-cm, audit-all-cm and ${AUDIT_LOG}); apply the same pattern to all occurrences noted (lines around the other create/grep pairs) so each grep only searches for the current run's ConfigMap entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/suites/configuration/audit-log.robot`:
- Around line 93-97: The teardown uses "Run Keywords Oc Delete ... AND
Remove Drop In MicroShift Config AND Restart MicroShift" which aborts
remaining steps if any earlier step fails; update each teardown block (the
instances using "Run Keywords" with "Oc Delete", "Remove Drop In MicroShift
Config", and "Restart MicroShift") to ensure all cleanup steps always run by
wrapping individual steps with a continue-on-failure variant, e.g. replace
direct calls with "Run Keyword And Ignore Error Oc Delete configmap
${cm_name} -n ${TEST_NS} --ignore-not-found" (or "Run Keyword And Continue On
Failure" if you want failure reported but not abort), then call "Run Keyword And
Ignore Error Remove Drop In MicroShift Config 10-audit" and "Run Keyword
And Ignore Error Restart MicroShift" so deletion, config removal, and restart
are attempted regardless of prior failures.
In `@test/suites/configuration/show-config.robot`:
- Around line 70-72: The test currently uses "Should Not Be Equal As Integers
180 ${config.etcd.memoryLimitMB}" which allows any non-180 value; change the
assertion to assert the exact baseline value fixed in suite setup by using
"Should Be Equal As Integers 256 ${config.etcd.memoryLimitMB}" after calling the
"Show Config" keyword so the ${config.etcd.memoryLimitMB} is verified equals
256.
---
Duplicate comments:
In `@test/resources/kustomize-test.resource`:
- Around line 30-32: The test's deletion guard around ${manifest_dir} is
insufficient because it only checks raw prefix and allows traversal or deletion
of the base root; update the test to normalize the path (resolve symlinks and
collapse .. components) before checks and assert that the normalized path is a
strict child of the intended base root (not equal to it) and not outside it;
replace the "Should Start With ${manifest_dir} /etc/microshift/" check with a
normalized containment assertion and add a check that the normalized
${manifest_dir} != /etc/microshift/ to prevent deleting the base-root, then only
run "Command Should Work rm -rf ${manifest_dir}" after these validations pass.
---
Nitpick comments:
In `@test/resources/kustomize-test.resource`:
- Line 12: Replace the unquoted mkdir invocation in the "Command Should Work"
test so ${manifest_dir} is passed as a single shell argument and options are
terminated; specifically modify the test line that currently uses "mkdir -p
${manifest_dir}" (the "Command Should Work" step) to call mkdir with a "--"
terminator and quote the ${manifest_dir} variable so paths with spaces, special
chars or a leading dash are handled safely.
In `@test/suites/configuration/audit-log.robot`:
- Around line 87-90: The tests use fixed ConfigMap names (e.g., audit-none-cm)
when calling Oc Create and then call the keyword Grep Audit Log Count against
${AUDIT_LOG}, which can match stale entries from prior runs and make zero-count
checks flaky; update the test to generate and use unique ConfigMap names for
each create (e.g., append a run-specific suffix/timestamp or ${TEST_RUN_ID}) and
pass that generated name into the Grep Audit Log Count call (references: the
variable ${cm_name}, the keyword Grep Audit Log Count, and the audit ConfigMap
names audit-none-cm, audit-default-cm, audit-write-cm, audit-all-cm and
${AUDIT_LOG}); apply the same pattern to all occurrences noted (lines around the
other create/grep pairs) so each grep only searches for the current run's
ConfigMap entries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: c59facd1-d9b0-41a0-a6b6-74ed0debdc09
📒 Files selected for processing (4)
test/resources/kustomize-test.resourcetest/suites/configuration/apiserver-readiness.robottest/suites/configuration/audit-log.robottest/suites/configuration/show-config.robot
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/suites/configuration/audit-log.robot (1)
192-192: ⚡ Quick winUse guarded grep rc handling instead of
|| true.Line 192 still uses
|| true, which can hide real grep errors. Align with the safer pattern already used elsewhere in this file (|| test $? -eq 1).Suggested patch
- ... grep -c '"${resource_name}"' ${AUDIT_LOG} || true + ... grep -c '"${resource_name}"' ${AUDIT_LOG} || test $? -eq 1🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/suites/configuration/audit-log.robot` at line 192, Replace the guarded grep exit handling for the command grep -c '"${resource_name}"' ${AUDIT_LOG} || true with the safer pattern used elsewhere: use "|| test $? -eq 1" so only the "no matches" exit code is allowed through while real grep errors still fail; update that exact grep invocation in the test/suites/configuration/audit-log.robot file accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/suites/configuration/kustomize-sources.robot`:
- Around line 88-95: The teardown uses a single "Run Keywords" chain which
short-circuits on the first failure and can skip final steps like "Restart
MicroShift"; fix by wrapping each individual teardown step (e.g., "Remove Drop
In MicroShift Config", each "Remove Manifest Directory
${MANIFEST_DIR_1}/${MANIFEST_DIR_2}", each "Oc Delete namespace ...", and
"Restart MicroShift") with "Run Keyword And Continue On Failure" so every
cleanup runs regardless of failures, replacing the current multi-step "Run
Keywords ... AND ..." pattern with separate continue-on-failure wrapped calls.
---
Nitpick comments:
In `@test/suites/configuration/audit-log.robot`:
- Line 192: Replace the guarded grep exit handling for the command grep -c
'"${resource_name}"' ${AUDIT_LOG} || true with the safer pattern used elsewhere:
use "|| test $? -eq 1" so only the "no matches" exit code is allowed through
while real grep errors still fail; update that exact grep invocation in the
test/suites/configuration/audit-log.robot file accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 2149c276-ed28-4393-81e7-b87db7d451e7
📒 Files selected for processing (2)
test/suites/configuration/audit-log.robottest/suites/configuration/kustomize-sources.robot
agullon
force-pushed
the
USHIFT-6743
branch
2 times, most recently
from
9276907 to
15f8105
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
…ed resource Move `Start MicroShift Without Waiting For Systemd Readiness` from lifecycle.robot into microshift-process.resource so it can be reused by the new apiserver-readiness.robot test. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> pre-commit.check-secrets: ENABLED
…words Add reusable keywords for deploying/cleaning up kustomize test manifests (Deploy Test Manifests, Remove Manifest Directory, ConfigMap Should Not Exist). Used by kustomize-sources.robot and drop-in-config.robot. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> pre-commit.check-secrets: ENABLED
Replaces Ginkgo test 62959: MicroShift must not read config from ~/.microshift/config.yaml. Verifies that only /etc/microshift/ config files are picked up by show-config --mode effective. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> pre-commit.check-secrets: ENABLED
Replaces Ginkgo test 63099: validates that MicroShift accepts log levels in any case variation (normal, Debug, TRACE, TraceAll). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> pre-commit.check-secrets: ENABLED
Replaces Ginkgo test 62987: verifies MicroShift only uses /var/lib/microshift/ for data and does not populate the legacy ~/.microshift/data/ path. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> pre-commit.check-secrets: ENABLED
Replaces Ginkgo test 76468: verifies drop-in config directory semantics — higher-numbered files override arrays (kustomizePaths, subjectAltNames) while maps merge across config.yaml and drop-ins. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> pre-commit.check-secrets: ENABLED
Replaces Ginkgo tests 63217 and 63298: tests empty, single, multiple, null, non-existent, and glob-pattern kustomizePaths configurations. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> pre-commit.check-secrets: ENABLED
Replaces Ginkgo tests 72334 and 72340: verifies all four audit profiles (None, Default, WriteRequestBodies, AllRequestBodies) by checking actual audit.log content, and tests log file rotation with maxFileSize and maxFiles constraints. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> pre-commit.check-secrets: ENABLED
Replaces Ginkgo test 55728: verifies the API server returns HTTP 429 when X-OpenShift-Internal-If-Not-Ready: reject header is sent during startup, preventing clients from using a partially initialized server. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> pre-commit.check-secrets: ENABLED
Contributor
Author
|
/retest |
1 similar comment
Contributor
Author
|
/retest |
agullon
force-pushed
the
USHIFT-6743
branch
2 times, most recently
from
b9ce680 to
1cd3f80
Compare
Deploy Test Manifests now includes a namespace.yaml resource so
the target namespace is created alongside the configmap.
Escape %{http_code} in curl command as \%{http_code} to prevent
Robot Framework from interpreting it as an environment variable.
Move audit-log.robot from configuration1 to configuration2 scenario
to avoid the 30-minute timeout.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
pre-commit.check-secrets: ENABLED
Contributor
Author
|
/retest |
Contributor
|
@agullon: The following tests failed, say
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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Migrates 9 Ginkgo tests from openshift-tests-private into Robot Framework tests in this repo.
Each new RF test replaces a specific Ginkgo test case:
logging.robot63099— make logging config more resilientkustomize-sources.robot63217— configurable manifest sources63298— manifest directory scanningaudit-log.robot72334— Make audit log policy configurable for MicroShift72340— Microshift Audit Log File Rotationdrop-in-config.robot76468— Drop-in configuration directoryapiserver-readiness.robot55728— Clients must not use an unready Kubernetes apiserverdata-dir.robot62987— Remove search logic for data directoryshow-config.robot(modified)62959— Remove search logic for configuration fileAlso promotes
Start MicroShift Without Waiting For Systemd Readinessfromlifecycle.robottomicroshift-process.resourceso it can be reused byapiserver-readiness.robot.All new files are under
test/suites/configuration/and are picked up by the existing CI scenario.Summary by CodeRabbit