Add OETF workflow-labels scenario tests - #1260
Merged
Merged
Conversation
Contributor
|
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 portable ChangesWorkflow labels scenario
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant WorkflowLabels
participant WorkflowAPI
participant WorkflowListAPI
WorkflowLabels->>WorkflowAPI: submit labeled workflow
WorkflowAPI-->>WorkflowLabels: return workflow metadata
WorkflowLabels->>WorkflowAPI: retrieve workflow labels
WorkflowLabels->>WorkflowListAPI: list with label filters
WorkflowListAPI-->>WorkflowLabels: return matching workflows
sequenceDiagram
participant WorkflowLabelsLifecycle
participant WorkflowAPI
participant WorkflowExecution
participant WorkflowListAPI
WorkflowLabelsLifecycle->>WorkflowAPI: submit labeled workflow
WorkflowAPI->>WorkflowExecution: execute workflow
WorkflowExecution-->>WorkflowAPI: report completed or failed state
WorkflowLabelsLifecycle->>WorkflowAPI: retrieve terminal workflow labels
WorkflowLabelsLifecycle->>WorkflowListAPI: filter terminal workflow by labels
WorkflowListAPI-->>WorkflowLabelsLifecycle: return labeled workflow
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
3 tasks
jiaenren
force-pushed
the
jiaenr/osmo-6501-d1-label-oetf
branch
2 times, most recently
from
August 5, 2026 00:11
eddf44d to
8380b0f
Compare
jiaenren
force-pushed
the
jiaenr/osmo-6501-d1-label-oetf
branch
from
August 5, 2026 18:26
8380b0f to
1ca3c68
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/scenarios/workflow_labels.py (1)
186-195: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winType the callback parameter.
_expect_rejectedalways invokescall, butAnyprevents static checking of that contract. UseCallable[[], object]and importCallable.As per coding guidelines, "Use strict typing in Python where it improves clarity."
Proposed correction
-from typing import Any, Dict, List, Optional +from typing import Any, Callable, Dict, List, Optional ... - def _expect_rejected(self, call: Any, *, status: int = 400, + def _expect_rejected(self, call: Callable[[], object], *, status: int = 400,🤖 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 `@test/scenarios/workflow_labels.py` around lines 186 - 195, Update the _expect_rejected callback annotation from Any to Callable[[], object], and add Callable to the typing imports. Keep the existing invocation and rejection assertions unchanged.Source: Coding guidelines
🤖 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 @.github/workflows/oetf-kind.yaml:
- Around line 170-173: Update the workflow-labels comment in the oetf-kind
workflow to accurately describe test/scenarios/workflow_labels.py coverage: API
label round-trip behavior, label persistence, and filtering. Remove claims that
it waits for workflow completion or verifies pod stamping.
In `@test/scenarios/workflow_labels.py`:
- Around line 248-257: Update test_malformed_labels_are_rejected to verify the
syntax-specific error code or message for every malformed-label submission
instead of accepting any HTTP 400. Configure policy as off where mutation is
permitted so badkey and emptyval reach label syntax validation, while preserving
the existing validation_only coverage for nested labels.
---
Nitpick comments:
In `@test/scenarios/workflow_labels.py`:
- Around line 186-195: Update the _expect_rejected callback annotation from Any
to Callable[[], object], and add Callable to the typing imports. Keep the
existing invocation and rejection assertions unchanged.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0b6f5e14-2b98-4e2b-8580-4153b8a4f0ce
📒 Files selected for processing (4)
.github/workflows/oetf-kind.yamltest/scenarios/BUILDtest/scenarios/workflow_labels.pytest/scenarios/workflow_labels.yaml
jiaenren
force-pushed
the
jiaenr/osmo-6501-d1-label-oetf
branch
from
August 10, 2026 21:08
1ca3c68 to
e0bf0d3
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/scenarios/workflow_labels.py (1)
185-194: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winType the callback contract.
Replace
AnywithCallable[[], object]. This method requires a zero-argument callable and ignores its return value.As per coding guidelines, “Use type annotations where they improve clarity and help catch errors.”
🤖 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 `@test/scenarios/workflow_labels.py` around lines 185 - 194, Update the _expect_rejected method’s call parameter annotation from Any to Callable[[], object], adding or using the appropriate Callable import. Preserve the zero-argument invocation and ignored return value behavior.Source: Coding guidelines
🤖 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 `@test/scenarios/workflow_labels.py`:
- Around line 260-270: Make the label-policy-dependent tests independent of the
baseline policy: in test/scenarios/workflow_labels.py ranges 260-270, 291-295,
and 316-332, configure an off policy before submissions, combining it with
pod_label_prefix for the prefix-validation test; when database policy is
immutable, skip unless the existing ConfigMap policy is compatible.
In `@test/scenarios/workflow_labels.yaml`:
- Around line 21-26: Update the lint configuration or workflow for the scenario
containing the Jinja blocks so templated YAML is rendered before YAMLlint
validation, or exclude templated scenario files from raw YAMLlint. Preserve
plain YAMLlint coverage for non-templated scenario files.
---
Nitpick comments:
In `@test/scenarios/workflow_labels.py`:
- Around line 185-194: Update the _expect_rejected method’s call parameter
annotation from Any to Callable[[], object], adding or using the appropriate
Callable import. Preserve the zero-argument invocation and ignored return value
behavior.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c96e3d63-1169-46db-be07-17e6bb7e61ed
📒 Files selected for processing (4)
.github/workflows/oetf-kind.yamltest/scenarios/BUILDtest/scenarios/workflow_labels.pytest/scenarios/workflow_labels.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- test/scenarios/BUILD
- .github/workflows/oetf-kind.yaml
jiaenren
changed the title
jiaenren
force-pushed
the
jiaenr/osmo-6501-d1-label-oetf
branch
3 times, most recently
from
August 11, 2026 00:52
59266ce to
3d424b2
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/scenarios/workflow_labels.py (1)
309-319: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMake these tests independent of the baseline label policy.
A baseline
enforcepolicy can reject the submissions before these tests reach their filter, round-trip, or prefix assertions. A non-curated key does not bypass a requiredteampolicy.team_acan also be absent from a target allow-list.
test/scenarios/workflow_labels.py#L309-L319: Set anoffpolicy in database mode before the filter submissions. Skip when ConfigMap-mode policy compatibility is unknown.test/scenarios/workflow_labels.py#L340-L344: Set anoffpolicy before the round-trip submission, or verify that the baseline policy permitsteam_a.test/scenarios/workflow_labels.py#L365-L388: Set theoffpolicy andpod_label_prefixtogether before the prefix submissions. Skip when ConfigMap-mode policy is incompatible.🤖 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 `@test/scenarios/workflow_labels.py` around lines 309 - 319, Make the label tests independent of baseline policy: in test/scenarios/workflow_labels.py#L309-L319, configure an off policy before filter submissions and skip when ConfigMap-mode compatibility is unknown; at `#L340-L344`, configure off before the round-trip submission or verify baseline acceptance of team_a; and at `#L365-L388`, configure off together with pod_label_prefix before prefix submissions and skip when ConfigMap-mode policy is incompatible.
🤖 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.
Duplicate comments:
In `@test/scenarios/workflow_labels.py`:
- Around line 309-319: Make the label tests independent of baseline policy: in
test/scenarios/workflow_labels.py#L309-L319, configure an off policy before
filter submissions and skip when ConfigMap-mode compatibility is unknown; at
`#L340-L344`, configure off before the round-trip submission or verify baseline
acceptance of team_a; and at `#L365-L388`, configure off together with
pod_label_prefix before prefix submissions and skip when ConfigMap-mode policy
is incompatible.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 99d55933-4b13-49fb-a721-c6eda28cae5d
📒 Files selected for processing (1)
test/scenarios/workflow_labels.py
KIND-safe policy/filter/round-trip checks (WorkflowLabels, gated on oetf:deploy_and_run --env kind) plus heavy run-to-terminal label- persistence checks (WorkflowLabelsLifecycle, runs on a real deployment, not KIND-gated). List queries pass all_pools so label filters resolve on multi-pool targets, not just single-pool KIND. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
jiaenren
force-pushed
the
jiaenr/osmo-6501-d1-label-oetf
branch
from
August 11, 2026 01:16
3d424b2 to
a81b2fb
Compare
jiaenren
changed the title
RyaliNvidia
approved these changes
Aug 11, 2026
elookpotts-nvidia
approved these changes
Aug 11, 2026
This was referenced Aug 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Issue - None
Adds an OETF end-to-end suite for the workflow-labels feature, split into two Bazel targets so the fast subset can gate PRs and the heavy subset runs against a real deployment. Both drive the feature purely through the OSMO API (a sandboxed OETF test has no in-cluster kubeconfig).
//test/scenarios:workflow-labels(WorkflowLabels, KIND-gated) — fast, API-only checks:warnsurfaceswarnings,enforcerejects with 400 and leaves no row,offaccepts anything.label, glob, andno_label.pod_label_prefix): validated against the merged key at submission (a bare key merges to a valid Kubernetes key; a key that already carries a prefix is rejected), and never leaks into the workflow API/list, which keep the bare keys.The policy tests mutate
labels_config, so they need DB-mode config andskipTeston a ConfigMap-mode target.//test/scenarios:workflow-labels-lifecycle(WorkflowLabelsLifecycle, not KIND-gated) — heavy end-to-end checks that run a labeled workflow to a terminal status and assert the labels survive the whole lifecycle:These run real pods to completion, so they're deliberately off the KIND PR gate (which runs only validation scenarios) and run against a real deployment instead.
Deployment-agnostic
The suite is meant to pass on any OSMO deployment:
all_poolsso label filters resolve when the submit pool (OETF_POOL) differs from the caller's profile pool — otherwise every filter comes back empty on a multi-pool target. No-op on single-pool KIND.skipTestwhen a target's baked policy would reject the generic label set. Curated-key enforcement is deployment-specific, so those checks live in the internal overlay, not here.Scope note (pod-object verification)
A literal in-cluster pod-label assertion is not reachable from OETF: scenarios run via
bazel testwith onlyOETF_URL+ auth +OETF_POOLforwarded, so the test can't reach the cluster with kubectl. The lifecycle target therefore asserts the label round-trip through a real run (submit → schedule → run → terminal, labels still echoed + filterable), not the literal pod label. The pod stamping itself (apply_workflow_labels/ prefix application) is covered by unit tests.Verification
bazel build //test/scenarios:workflow-labels //test/scenarios:workflow-labels-lifecyclepass locally.oetf:deploy_and_run --env kind) runs theworkflow-labelstarget on this PR.Ran 7, skipped 4— the config-mutation tests skip on ConfigMap; filters + round-trip pass viaall_pools), and the lifecycle target passes (Ran 2— the COMPLETED and FAILED runs both keep their labels).Checklist
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests