USHIFT-6745: Port OTP etcd test cases to Robot Framework#6415
USHIFT-6745: Port OTP etcd test cases to Robot Framework#6415agullon wants to merge 1 commit intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@agullon: This pull request references USHIFT-6745 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 "4.22.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. |
|
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:
WalkthroughSuite Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[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
|
@agullon: This pull request references USHIFT-6745 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 "4.22.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. |
|
/jira refresh |
|
@agullon: This pull request references USHIFT-6745 which is a valid jira issue. 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/suites/standard1/etcd.robot (1)
148-152: Shell quoting in JSON parsing could break on edge cases.Using
echo '${output}'is fragile if the JSON ever contains literal single quotes. Consider using a heredoc or piping directly.♻️ Suggested fix using printf
Get Etcd Database Size [Documentation] Return the current etcd database size in bytes ${output}= Command Should Work ${ETCDCTL_CMD} endpoint status --write-out\=json - ${size}= Command Should Work echo '${output}' | python3 -c "import sys,json; print(json.load(sys.stdin)[0]['Status']['dbSize'])" + ${size}= Command Should Work printf '%s' '${output}' | python3 -c "import sys,json; print(json.load(sys.stdin)[0]['Status']['dbSize'])" RETURN ${size}Alternatively, combine into a single command to avoid intermediate shell expansion entirely.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/suites/standard1/etcd.robot` around lines 148 - 152, The step "Get Etcd Database Size" is fragile because it uses echo '${output}' which can break if the JSON contains single quotes; change the pipeline to avoid shell interpolation: take the ${output} captured by Command Should Work and pipe it directly into the Python JSON parser (or use printf/ heredoc) so you don't rely on echo with quoted content—update the lines that set ${output} and ${size} (the Command Should Work invocations) to feed the JSON safely into python3 (or a single combined command) instead of using echo '${output}'.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/suites/standard1/etcd.robot`:
- Around line 148-152: The step "Get Etcd Database Size" is fragile because it
uses echo '${output}' which can break if the JSON contains single quotes; change
the pipeline to avoid shell interpolation: take the ${output} captured by
Command Should Work and pipe it directly into the Python JSON parser (or use
printf/ heredoc) so you don't rely on echo with quoted content—update the lines
that set ${output} and ${size} (the Command Should Work invocations) to feed the
JSON safely into python3 (or a single combined command) instead of using echo
'${output}'.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: b9d3caef-6347-4631-b2a2-f0fec3604252
📒 Files selected for processing (1)
test/suites/standard1/etcd.robot
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/suites/standard1/etcd.robot (1)
134-146: Consider adding checksum verification for downloaded etcdctl binary.Downloading from GitHub releases without verifying a checksum is acceptable for test environments but reduces supply-chain assurance. If this is intentional for simplicity, a brief comment explaining the tradeoff would help future maintainers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/suites/standard1/etcd.robot` around lines 134 - 146, The test "Install Etcdctl If Missing" currently downloads and extracts etcdctl via the curl|tar pipeline (the Command Should Work that uses https://github.com/etcd-io/etcd/releases/download/v${etcd_ver}/etcd-v${etcd_ver}-linux-${arch_suffix}.tar.gz) without verifying integrity; add checksum verification by also downloading the corresponding release checksum (or GPG signature) for v${etcd_ver}, validating the tarball before extracting, and fail the test if verification fails; alternatively, if you intentionally omit verification for simplicity, add a short comment above the download (near the ${etcd_ver} and ${arch_suffix} logic) documenting the tradeoff and rationale.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/suites/standard1/etcd.robot`:
- Around line 134-146: The test "Install Etcdctl If Missing" currently downloads
and extracts etcdctl via the curl|tar pipeline (the Command Should Work that
uses
https://github.com/etcd-io/etcd/releases/download/v${etcd_ver}/etcd-v${etcd_ver}-linux-${arch_suffix}.tar.gz)
without verifying integrity; add checksum verification by also downloading the
corresponding release checksum (or GPG signature) for v${etcd_ver}, validating
the tarball before extracting, and fail the test if verification fails;
alternatively, if you intentionally omit verification for simplicity, add a
short comment above the download (near the ${etcd_ver} and ${arch_suffix} logic)
documenting the tradeoff and rationale.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 9a6eccc1-5b6f-48db-b7ba-4e8659c12fb9
📒 Files selected for processing (2)
test/suites/standard1/etcd.robottest/suites/standard2/validate-certificate-rotation.robot
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/suites/standard1/etcd.robot (1)
148-152: Consider heredoc for robustness against special characters.The
echo '${output}'pattern breaks if JSON ever contains single quotes. While unlikely with etcdctl output, a heredoc is more robust.Alternative using heredoc
Get Etcd Database Size [Documentation] Return the current etcd database size in bytes ${output}= Command Should Work ${ETCDCTL_CMD} endpoint status --write-out\=json ${size}= Command Should Work - ... echo '${output}' | python3 -c "import sys,json; print(json.load(sys.stdin)[0]['Status']['dbSize'])" + ... python3 -c "import json; print(json.loads('''${output}''')[0]['Status']['dbSize'])" RETURN ${size}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/suites/standard1/etcd.robot` around lines 148 - 152, The current step "Get Etcd Database Size" pipes echo '${output}' into python3 which will break if the JSON contains single quotes; instead, pass the ${output} variable into python3 via a heredoc to avoid shell quoting issues: update the second Command Should Work call that runs python3 -c "import sys,json; print(...)" so it consumes stdin from a quoted here-document containing ${output} (e.g. <<'EOF' ... EOF), preserving the python one-liner and extracting ['Status']['dbSize'] from the JSON.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/suites/standard1/etcd.robot`:
- Around line 148-152: The current step "Get Etcd Database Size" pipes echo
'${output}' into python3 which will break if the JSON contains single quotes;
instead, pass the ${output} variable into python3 via a heredoc to avoid shell
quoting issues: update the second Command Should Work call that runs python3 -c
"import sys,json; print(...)" so it consumes stdin from a quoted here-document
containing ${output} (e.g. <<'EOF' ... EOF), preserving the python one-liner and
extracting ['Status']['dbSize'] from the JSON.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: bf7893cb-788b-4156-bd4a-7d8c23a11a5f
📒 Files selected for processing (2)
test/suites/standard1/etcd.robottest/suites/standard2/validate-certificate-rotation.robot
agullon
force-pushed
the
USHIFT-6745
branch
2 times, most recently
from
695ac1d to
a0a6d3f
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/suites/standard1/etcd.robot`:
- Around line 135-148: The test keyword "Install Etcdctl If Missing" currently
downloads and executes an unverified etcdctl from GitHub at runtime (using
curl/tar and microshift-etcd to detect version) which breaks in disconnected CI
and skips artifact verification; change the test to stop performing the direct
curl/tar fetch and instead use a pre-staged etcdctl from test fixtures (replace
the runtime download logic that references ${ETCDCTL_BIN}, ${etcd_ver},
microshift-etcd and the curl/tar bash command) or, if dynamic fetch is
absolutely required, add a checksum verification step against a known-good hash
before making ${ETCDCTL_BIN} executable and running it; ensure the keyword
resolves ${ETCDCTL_BIN} to the fixture path and remove sudo curl/tar usage so no
outbound fetch occurs during test execution.
- Around line 121-128: The test "Etcd Scope Is Inactive" currently uses "Should
Not Be Equal As Strings ${stdout.strip()} active" which also allows undesired
states like "failed"; update the assertion to explicitly allow only the expected
states by replacing that line with an explicit membership check such as using
"Should Be One Of ${stdout.strip()} inactive unknown" (or equivalent
Robot keyword) so the output from Execute Command (systemctl is-active
${ETCD_SYSTEMD_UNIT}) is restricted to only "inactive" or "unknown".
In `@test/suites/standard2/validate-certificate-rotation.robot`:
- Around line 45-46: The current test uses the keyword "Certificate Should Be
Valid For Current Time" which only asserts notBefore <= now; update the test to
also assert that now < notAfter (i.e., certificate not expired) after "Verify
Remote File Exists With Sudo ${cert_file}". Either extend or replace the keyword
call with one that checks both notBefore and notAfter, or add a new assertion
(e.g., "Certificate Not Expired" or "Certificate Should Be Valid NowAndLater")
that parses ${cert_file} and verifies now < notAfter; reference the existing
keyword name "Certificate Should Be Valid For Current Time" when implementing
the combined check so the helper is updated consistently.
🪄 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), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 93980c3c-3299-4d00-8339-23bea32dd562
📒 Files selected for processing (2)
test/suites/standard1/etcd.robottest/suites/standard2/validate-certificate-rotation.robot
agullon
force-pushed
the
USHIFT-6745
branch
2 times, most recently
from
d38e98d to
c275232
Compare
Port MicroShiftOnly OTP etcd test cases to Robot Framework as part of USHIFT-6690. Add the following tests to existing suite files: etcd.robot (standard1): - Etcd Database Defragment Manually (OCP-71790): run etcdctl defrag and verify db size does not grow - Etcd Runs As Transient Systemd Scope Unit (OCP-62738): verify microshift-etcd.scope is running, transient, and has correct systemd wiring (BindsTo/Before microshift.service) - Etcd Scope Follows MicroShift Lifecycle (OCP-60945): verify etcd scope stops/starts with MicroShift validate-certificate-rotation.robot (standard2): - Manual Rotation Of Etcd Signer Certs (OCP-75224): delete etcd signer certs, restart MicroShift, verify all 4 certs are regenerated with valid dates and different fingerprints Also improves Restore System Date to skip when chronyd is already active, preventing timeout when the etcd cert test runs without the clock change test. USHIFT-6745 pre-commit.check-secrets: ENABLED
|
/test e2e-aws-tests-release |
|
/retest |
|
@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. |
2 participants
Summary
standard1/etcd.robot: etcd defragmentation (OCP-71790), transient scope verification (OCP-62738), and lifecycle tracking (OCP-60945)standard2/validate-certificate-rotation.robot(OCP-75224)Restore System Daterobustness for shared suite executionOTP coverage mapping
etcd.robotetcd.robotetcd.robotvalidate-certificate-rotation.robotetcd.robotUSHIFT-6745