Fix access policies not updating status when no job #532
Conversation
WalkthroughThe pull request introduces significant updates to the Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- internal/controllers/skipjob.go (2 hunks)
- tests/skipjob/access-policy-job/chainsaw-test.yaml (1 hunks)
- tests/skipjob/access-policy-job/skipjob-cron-assert.yaml (1 hunks)
- tests/skipjob/access-policy-job/status-ready-no-job-assert.yaml (1 hunks)
- tests/skipjob/access-policy-job/status-ready-no-job.yaml (1 hunks)
Additional comments not posted (4)
tests/skipjob/access-policy-job/status-ready-no-job.yaml (1)
1-19: Configuration appears correct and well-structured.This new YAML file for
SKIPJobnamedstatus-ready-no-jobis well-structured and follows Kubernetes configuration standards. The access policy, image, cron, and job settings are clearly defined, which should work as expected in the Kubernetes environment.tests/skipjob/access-policy-job/chainsaw-test.yaml (1)
37-41: New test block added correctly.The addition of the
tryblock withapplyandassertoperations forstatus-ready-no-job.yamlandstatus-ready-no-job-assert.yamlrespectively is correctly implemented. This enhances the test coverage by including checks for the status readiness without a job, which aligns with the PR objectives.tests/skipjob/access-policy-job/status-ready-no-job-assert.yaml (1)
1-45: Assertion configuration appears correct and comprehensive.This new YAML file for asserting the status of
status-ready-no-jobSKIPJob is well-structured and comprehensive. It includes detailed conditions and subresources statuses, which should effectively validate the state of the SKIPJob as intended.tests/skipjob/access-policy-job/skipjob-cron-assert.yaml (1)
27-27: Proper placement and usage of theaccessPoliciesfield.The addition of
accessPoliciesunder thestatussection is correctly implemented. Ensure that the valueReadyis consistent with other status values used across your Kubernetes configurations.Run the following script to verify consistency of status values across Kubernetes configurations:
Verification successful
Consistent usage of the
accessPolicies: Readystatus value.The status value
ReadyforaccessPoliciesis consistently used across multiple YAML files in the codebase, indicating it is a standard and accepted status value. No inconsistencies were found with other status values.
- Files with
accessPolicies: Readyinclude:
tests/skipjob/access-policy-job/skipjob-cron-assert.yamltests/skipjob/access-policy-job/application-assert.yamltests/application/access-policy/advanced-assert.yamlScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistency of status values across Kubernetes configurations. # Test: Search for status values in Kubernetes YAML files. Expect: Consistent usage of status values. rg --type yaml -A 5 $'status:'Length of output: 34295
| if len(jobList.Items) == 0 { | ||
| skipJob.Status.Conditions = []v1.Condition{ | ||
| r.getConditionFailed(skipJob, v1.ConditionFalse, nil), | ||
| r.getConditionRunning(skipJob, v1.ConditionFalse), | ||
| r.getConditionFinished(skipJob, v1.ConditionFalse), | ||
| } |
There was a problem hiding this comment.
Enhanced handling of empty job lists in updateConditions.
The new logic to handle cases where the job list is empty is correctly implemented. This change ensures that the job's status is accurately reported even when no jobs are present.
Consider adding unit tests to cover this new logic to ensure it behaves as expected under various scenarios.
Would you like assistance in writing these unit tests? I can help draft some test cases or open a GitHub issue to track this task.
Summary by CodeRabbit
New Features
Bug Fixes
Tests