CNTRLPLANE-3428: Adding TLS profile observed test cases#79416
Conversation
|
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 two optional one-off CI jobs to run the openshift/tls-observed-config test suite: one on ChangesTLS observed-config CI jobs
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
🚥 Pre-merge checks | ✅ 12✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@ci-operator/config/openshift/tls-scanner/openshift-tls-scanner-main.yaml`:
- Around line 159-175: Add a reporter_config block to the new periodic jobs
periodic-tls-observed-config and periodic-tls-observed-config-hypershift so they
match the other periodic jobs' notification behavior; copy the same
reporter_config used by the existing periodic jobs (the Slack notification to
`#forum-case` with notify_on: ["failure"] and notify: ["slack"]) and insert it
under each job's top-level keys (alongside interval/steps/workflow) to ensure
failures are reported to the team.
In
`@ci-operator/config/openshift/tls-scanner/openshift-tls-scanner-release-4.22.yaml`:
- Around line 128-144: The two new periodic jobs periodic-tls-observed-config
and periodic-tls-observed-config-hypershift are missing reporter_config
sections; add a reporter_config block to each job (matching the pattern used by
periodic-default-tls and the main branch) that configures Slack notifications to
`#forum-case` (include channel, send and template keys as used elsewhere) so both
jobs report consistently; update the reporter_config under the job definitions
for periodic-tls-observed-config and periodic-tls-observed-config-hypershift.
🪄 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: 8d34c314-713c-479f-98d5-a8969c5306b1
⛔ Files ignored due to path filters (3)
ci-operator/jobs/openshift/tls-scanner/openshift-tls-scanner-main-presubmits.yamlis excluded by!ci-operator/jobs/**ci-operator/jobs/openshift/tls-scanner/openshift-tls-scanner-release-4.22-periodics.yamlis excluded by!ci-operator/jobs/**ci-operator/jobs/openshift/tls-scanner/openshift-tls-scanner-release-4.22-presubmits.yamlis excluded by!ci-operator/jobs/**
📒 Files selected for processing (2)
ci-operator/config/openshift/tls-scanner/openshift-tls-scanner-main.yamlci-operator/config/openshift/tls-scanner/openshift-tls-scanner-release-4.22.yaml
| - as: periodic-tls-observed-config | ||
| interval: 72h | ||
| steps: | ||
| cluster_profile: openshift-org-aws | ||
| env: | ||
| COMPUTE_NODE_TYPE: m5.4xlarge | ||
| TEST_SUITE: openshift/tls-observed-config-ocp | ||
| test: | ||
| - ref: openshift-e2e-test | ||
| workflow: ipi-aws | ||
| - as: periodic-tls-observed-config-hypershift | ||
| interval: 72h | ||
| steps: | ||
| cluster_profile: hypershift-aws | ||
| env: | ||
| TEST_SUITE: openshift/tls-observed-config-hypershift | ||
| workflow: hypershift-aws-conformance |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Missing reporter_config for new periodic jobs.
The new periodic jobs periodic-tls-observed-config and periodic-tls-observed-config-hypershift lack reporter_config sections, while all other periodic jobs in this file (lines 79-139) include Slack notifications to #forum-case. This inconsistency means failures in these new periodic jobs won't be reported to the team.
📢 Proposed fix to add reporter_config
- as: periodic-tls-observed-config
interval: 72h
+ reporter_config:
+ channel: '`#forum-case`'
+ job_states_to_report:
+ - success
+ - failure
+ - error
+ report_template: '{{if eq .Status.State "success"}} :white_check_mark: Job *{{.Spec.Job}}*
+ ended with *{{.Status.State}}*. <{{.Status.URL}}|View logs> {{else}} :warning:
+ Job *{{.Spec.Job}}* ended with *{{.Status.State}}*. <{{.Status.URL}}|View logs>
+ {{end}}'
steps:Apply the same for periodic-tls-observed-config-hypershift at line 169.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - as: periodic-tls-observed-config | |
| interval: 72h | |
| steps: | |
| cluster_profile: openshift-org-aws | |
| env: | |
| COMPUTE_NODE_TYPE: m5.4xlarge | |
| TEST_SUITE: openshift/tls-observed-config-ocp | |
| test: | |
| - ref: openshift-e2e-test | |
| workflow: ipi-aws | |
| - as: periodic-tls-observed-config-hypershift | |
| interval: 72h | |
| steps: | |
| cluster_profile: hypershift-aws | |
| env: | |
| TEST_SUITE: openshift/tls-observed-config-hypershift | |
| workflow: hypershift-aws-conformance | |
| - as: periodic-tls-observed-config | |
| interval: 72h | |
| reporter_config: | |
| channel: '`#forum-case`' | |
| job_states_to_report: | |
| - success | |
| - failure | |
| - error | |
| report_template: '{{if eq .Status.State "success"}} :white_check_mark: Job *{{.Spec.Job}}* | |
| ended with *{{.Status.State}}*. <{{.Status.URL}}|View logs> {{else}} :warning: | |
| Job *{{.Spec.Job}}* ended with *{{.Status.State}}*. <{{.Status.URL}}|View logs> | |
| {{end}}' | |
| steps: | |
| cluster_profile: openshift-org-aws | |
| env: | |
| COMPUTE_NODE_TYPE: m5.4xlarge | |
| TEST_SUITE: openshift/tls-observed-config-ocp | |
| test: | |
| - ref: openshift-e2e-test | |
| workflow: ipi-aws | |
| - as: periodic-tls-observed-config-hypershift | |
| interval: 72h | |
| reporter_config: | |
| channel: '`#forum-case`' | |
| job_states_to_report: | |
| - success | |
| - failure | |
| - error | |
| report_template: '{{if eq .Status.State "success"}} :white_check_mark: Job *{{.Spec.Job}}* | |
| ended with *{{.Status.State}}*. <{{.Status.URL}}|View logs> {{else}} :warning: | |
| Job *{{.Spec.Job}}* ended with *{{.Status.State}}*. <{{.Status.URL}}|View logs> | |
| {{end}}' | |
| steps: | |
| cluster_profile: hypershift-aws | |
| env: | |
| TEST_SUITE: openshift/tls-observed-config-hypershift | |
| workflow: hypershift-aws-conformance |
🤖 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 `@ci-operator/config/openshift/tls-scanner/openshift-tls-scanner-main.yaml`
around lines 159 - 175, Add a reporter_config block to the new periodic jobs
periodic-tls-observed-config and periodic-tls-observed-config-hypershift so they
match the other periodic jobs' notification behavior; copy the same
reporter_config used by the existing periodic jobs (the Slack notification to
`#forum-case` with notify_on: ["failure"] and notify: ["slack"]) and insert it
under each job's top-level keys (alongside interval/steps/workflow) to ensure
failures are reported to the team.
| - as: periodic-tls-observed-config | ||
| interval: 72h | ||
| steps: | ||
| cluster_profile: openshift-org-aws | ||
| env: | ||
| COMPUTE_NODE_TYPE: m5.4xlarge | ||
| TEST_SUITE: openshift/tls-observed-config-ocp | ||
| test: | ||
| - ref: openshift-e2e-test | ||
| workflow: ipi-aws | ||
| - as: periodic-tls-observed-config-hypershift | ||
| interval: 72h | ||
| steps: | ||
| cluster_profile: hypershift-aws | ||
| env: | ||
| TEST_SUITE: openshift/tls-observed-config-hypershift | ||
| workflow: hypershift-aws-conformance |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Missing reporter_config for new periodic jobs.
The new periodic jobs periodic-tls-observed-config and periodic-tls-observed-config-hypershift lack reporter_config sections. Note that this file already has inconsistent reporting: periodic-default-tls (line 99) includes Slack notifications, while periodic-tls13-conformance (line 118) does not. For consistency with periodic-default-tls and the main branch configuration (where all periodic jobs report to #forum-case), consider adding reporter_config to these new jobs.
📢 Proposed fix to add reporter_config
- as: periodic-tls-observed-config
interval: 72h
+ reporter_config:
+ channel: '`#forum-case`'
+ job_states_to_report:
+ - success
+ - failure
+ - error
+ report_template: '{{if eq .Status.State "success"}} :white_check_mark: Job *{{.Spec.Job}}*
+ ended with *{{.Status.State}}*. <{{.Status.URL}}|View logs> {{else}} :warning:
+ Job *{{.Spec.Job}}* ended with *{{.Status.State}}*. <{{.Status.URL}}|View logs>
+ {{end}}'
steps:Apply the same for periodic-tls-observed-config-hypershift at line 138.
🤖 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
`@ci-operator/config/openshift/tls-scanner/openshift-tls-scanner-release-4.22.yaml`
around lines 128 - 144, The two new periodic jobs periodic-tls-observed-config
and periodic-tls-observed-config-hypershift are missing reporter_config
sections; add a reporter_config block to each job (matching the pattern used by
periodic-default-tls and the main branch) that configures Slack notifications to
`#forum-case` (include channel, send and template keys as used elsewhere) so both
jobs report consistently; update the reporter_config under the job definitions
for periodic-tls-observed-config and periodic-tls-observed-config-hypershift.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
ci-operator/config/openshift/origin/openshift-origin-main.yaml (2)
858-865: Consider enabling observers for consistency.Many similar e2e test configurations in this file enable the
observers-resource-watchobserver (e.g., lines 103-105, 128-130, 252-254). Consider adding observers to this test for consistency and enhanced monitoring capabilities.Optional enhancement
steps: cluster_profile: openshift-org-aws env: COMPUTE_NODE_TYPE: m5.4xlarge TEST_SUITE: openshift/tls-observed-config + observers: + enable: + - observers-resource-watch test: - ref: openshift-e2e-test workflow: ipi-aws🤖 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 `@ci-operator/config/openshift/origin/openshift-origin-main.yaml` around lines 858 - 865, Add the observers-resource-watch observer to the test step for the TEST_SUITE "openshift/tls-observed-config" so it matches other e2e configs: update the steps/test block that references "openshift-e2e-test" to include an observers list (or observers-resource-watch entry) alongside existing env/cluster_profile settings to enable the observer for this workflow (ipi-aws) and ensure consistent monitoring behavior.
868-872: Consider enabling observers for consistency.Similar to the standard AWS test, consider adding the
observers-resource-watchobserver to this HyperShift test configuration for consistency with other e2e tests in this file.Optional enhancement
steps: cluster_profile: hypershift-aws env: TEST_SUITE: openshift/tls-observed-config + observers: + enable: + - observers-resource-watch workflow: hypershift-aws-conformance🤖 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 `@ci-operator/config/openshift/origin/openshift-origin-main.yaml` around lines 868 - 872, Add the observers-resource-watch observer to this HyperShift test job so it matches other e2e tests: update the job definition that contains steps/cluster_profile: hypershift-aws, env/TEST_SUITE: openshift/tls-observed-config, and workflow: hypershift-aws-conformance to include the observers-resource-watch observer entry (same placement as other AWS jobs in this file) so the test runs with resource-watch observers enabled for consistency.
🤖 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 `@ci-operator/config/openshift/origin/openshift-origin-main.yaml`:
- Around line 866-872: Add the explicit always_run: false field to the periodic
job definition for the job identified by as:
periodic-tls-observed-config-hypershift; locate the block that sets interval:
72h, steps: { cluster_profile: hypershift-aws, env: TEST_SUITE:
openshift/tls-observed-config, workflow: hypershift-aws-conformance } and insert
always_run: false alongside those fields to match other entries and make the
job's execution behavior explicit.
- Around line 856-865: The periodic job definition for
"periodic-tls-observed-config" is missing an explicit always_run setting; add
always_run: false under the job's steps (alongside cluster_profile, env,
TEST_SUITE, and test) so the job block for periodic-tls-observed-config
explicitly includes always_run: false for consistency with other entries and to
avoid ambiguous behavior.
---
Nitpick comments:
In `@ci-operator/config/openshift/origin/openshift-origin-main.yaml`:
- Around line 858-865: Add the observers-resource-watch observer to the test
step for the TEST_SUITE "openshift/tls-observed-config" so it matches other e2e
configs: update the steps/test block that references "openshift-e2e-test" to
include an observers list (or observers-resource-watch entry) alongside existing
env/cluster_profile settings to enable the observer for this workflow (ipi-aws)
and ensure consistent monitoring behavior.
- Around line 868-872: Add the observers-resource-watch observer to this
HyperShift test job so it matches other e2e tests: update the job definition
that contains steps/cluster_profile: hypershift-aws, env/TEST_SUITE:
openshift/tls-observed-config, and workflow: hypershift-aws-conformance to
include the observers-resource-watch observer entry (same placement as other AWS
jobs in this file) so the test runs with resource-watch observers enabled for
consistency.
🪄 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: 571513e2-30c9-4347-af8f-51c5729dac78
⛔ Files ignored due to path filters (1)
ci-operator/jobs/openshift/origin/openshift-origin-main-periodics.yamlis excluded by!ci-operator/jobs/**
📒 Files selected for processing (2)
ci-operator/config/openshift/origin/openshift-origin-main.yamlcore-services/prow/02_config/openshift-virtualization/virt-platform-autopilot/_pluginconfig.yaml
| - as: periodic-tls-observed-config | ||
| interval: 72h | ||
| steps: | ||
| cluster_profile: openshift-org-aws | ||
| env: | ||
| COMPUTE_NODE_TYPE: m5.4xlarge | ||
| TEST_SUITE: openshift/tls-observed-config | ||
| test: | ||
| - ref: openshift-e2e-test | ||
| workflow: ipi-aws |
There was a problem hiding this comment.
Add always_run: false for consistency and clarity.
All other test entries in this file explicitly set always_run: false. While periodic tests with an interval may not run on PRs by default, it's better to be explicit to avoid unintended behavior and maintain consistency with the rest of the configuration.
Proposed fix
- as: periodic-tls-observed-config
+ always_run: false
interval: 72h
steps:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - as: periodic-tls-observed-config | |
| interval: 72h | |
| steps: | |
| cluster_profile: openshift-org-aws | |
| env: | |
| COMPUTE_NODE_TYPE: m5.4xlarge | |
| TEST_SUITE: openshift/tls-observed-config | |
| test: | |
| - ref: openshift-e2e-test | |
| workflow: ipi-aws | |
| - as: periodic-tls-observed-config | |
| always_run: false | |
| interval: 72h | |
| steps: | |
| cluster_profile: openshift-org-aws | |
| env: | |
| COMPUTE_NODE_TYPE: m5.4xlarge | |
| TEST_SUITE: openshift/tls-observed-config | |
| test: | |
| - ref: openshift-e2e-test | |
| workflow: ipi-aws |
🤖 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 `@ci-operator/config/openshift/origin/openshift-origin-main.yaml` around lines
856 - 865, The periodic job definition for "periodic-tls-observed-config" is
missing an explicit always_run setting; add always_run: false under the job's
steps (alongside cluster_profile, env, TEST_SUITE, and test) so the job block
for periodic-tls-observed-config explicitly includes always_run: false for
consistency with other entries and to avoid ambiguous behavior.
| - as: periodic-tls-observed-config-hypershift | ||
| interval: 72h | ||
| steps: | ||
| cluster_profile: hypershift-aws | ||
| env: | ||
| TEST_SUITE: openshift/tls-observed-config | ||
| workflow: hypershift-aws-conformance |
There was a problem hiding this comment.
Add always_run: false for consistency and clarity.
Same issue as the previous test entry: all other test entries in this file explicitly set always_run: false. Add this field to be explicit about the test's execution behavior and maintain consistency.
Proposed fix
- as: periodic-tls-observed-config-hypershift
+ always_run: false
interval: 72h
steps:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - as: periodic-tls-observed-config-hypershift | |
| interval: 72h | |
| steps: | |
| cluster_profile: hypershift-aws | |
| env: | |
| TEST_SUITE: openshift/tls-observed-config | |
| workflow: hypershift-aws-conformance | |
| - as: periodic-tls-observed-config-hypershift | |
| always_run: false | |
| interval: 72h | |
| steps: | |
| cluster_profile: hypershift-aws | |
| env: | |
| TEST_SUITE: openshift/tls-observed-config | |
| workflow: hypershift-aws-conformance |
🤖 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 `@ci-operator/config/openshift/origin/openshift-origin-main.yaml` around lines
866 - 872, Add the explicit always_run: false field to the periodic job
definition for the job identified by as:
periodic-tls-observed-config-hypershift; locate the block that sets interval:
72h, steps: { cluster_profile: hypershift-aws, env: TEST_SUITE:
openshift/tls-observed-config, workflow: hypershift-aws-conformance } and insert
always_run: false alongside those fields to match other entries and make the
job's execution behavior explicit.
gangwgr
force-pushed
the
tls-ci-observer
branch
2 times, most recently
from
ecf44af to
7d842bd
Compare
Add periodic-tls-observed-config and periodic-tls-observed-config-hypershift jobs to openshift/origin for main branch. Co-authored-by: Cursor <cursoragent@cursor.com>
|
[REHEARSALNOTIFIER]
Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
|
/approve |
openshift-ci
Bot
added
the
approved
gangwgr
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@gangwgr: This pull request references CNTRLPLANE-3428 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 "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. |
|
/pj-rehearse |
|
@gangwgr: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gangwgr, ingvagabund, smg247 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 |
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
|
/unhold |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
/pj-rehearse ack |
|
@gangwgr: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
openshift-merge-bot
Bot
added
the
rehearsals-ack
|
@gangwgr: The following test 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. |
openshift-merge-bot
Bot
merged commit acd6a07
into
openshift:main
Add periodic-tls-observed-config and periodic-tls-observed-config-hypershift jobs to openshift/origin for main branch. Co-authored-by: Cursor <cursoragent@cursor.com>
4 participants
Summary
Test plan
make updategenerates correct Prow job configsThis PR updates the OpenShift CI configuration (ci-operator configs under openshift/origin) to add TLS-profile "observed" test entries and required Hypershift base images used by the Hypershift AWS conformance workflow.
What changed, in practical terms
Notes on periodic/presubmit jobs
Impact
Files/areas affected (practical)