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. |
📝 WalkthroughWalkthroughAdds a new Robot Framework test suite that clones the tls-scanner repo, ensures cluster-reader RBAC, deploys a host-mode scanner Job to MicroShift, waits for completion, collects artifacts ( 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🎯 2 (Simple) | ⏱️ ~10 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.githubusercontent.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: 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. |
Signed-off-by: Evgeny Slutsky <eslutsky@redhat.com>
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.
2 participants
Summary by CodeRabbit