chore: add operator integration tests workflow#246
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughA new GitHub Actions workflow Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Supply chain surface (CWE-494, CWE-829): The caller passes Verify that The policy exclusion ( 🚥 Pre-merge checks | ✅ 4 | ❌ 6❌ Failed checks (6 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
/hold I'll need to update this PR with the new SHA once opendatahub-io/mlflow-operator#123 is merged |
kramaranya
force-pushed
the
operator-integration-tests
branch
from
ce213f7 to
cb48d29
Compare
kramaranya
force-pushed
the
operator-integration-tests
branch
2 times, most recently
from
28fc92f to
306cf9d
Compare
| jobs_without_timeout(jobs) := {job_id | | ||
| some job_id, job in jobs | ||
| not job["timeout-minutes"] | ||
| not job.uses |
There was a problem hiding this comment.
needed this for reusable workflow jobs, since they can't set timeout-minutes
| @@ -105,7 +105,7 @@ def _iter_actions(path: Path) -> Iterator[ActionRef]: | |||
| for lineno, line in enumerate(f, start=1): | |||
| if m := _USES_RE.match(line): | |||
| action = m.group("action") | |||
| if not action.startswith("./"): | |||
| if not action.startswith("./") and "/.github/workflows/" not in action: | |||
There was a problem hiding this comment.
needed so the pin check doesn't misread our reusable workflow call as a regular action
|
|
||
| push: | ||
| branches: | ||
| - master |
There was a problem hiding this comment.
Could you add rhoai-* and main as well so sync to RHDS doesn't require a diff.
|
|
||
| jobs: | ||
| integration-tests: | ||
| uses: opendatahub-io/mlflow-operator/.github/workflows/integration-tests.yml@d76a54cb14617d048ed64c3936fc4702efcc4636 |
There was a problem hiding this comment.
Is there a way to make this dynamic to use the matching release branch?
There was a problem hiding this comment.
I don’t think we can make uses: dynamic, GitHub requires a static value as per https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-syntax#jobsjob_iduses
I think reusing this workflow downstream should still be fine, wdyt?
There was a problem hiding this comment.
I think this exposes a limitation of reusable workflows since jobs.<job_id>.uses has to be a fully static reference. Ideally, we want to be running the MLflow operator tests from the same GitHub org and same target branch (exception being master vs main in ODH).
We may need to switch to either:
- a normal job that triggers
mlflow-operatorviaworkflow_dispatch/repository_dispatch, passing the org + branch dynamically, or - a normal job that checks out
${{ github.repository_owner }}/mlflow-operatorat the matching branch and runs the integration test entrypoint directly.
Both of those support dynamic org/branch selection at runtime, while uses: does not. What are your thoughts?
kramaranya
force-pushed
the
operator-integration-tests
branch
from
306cf9d to
83b9729
Compare
![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
🤖 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 `@dev/check_action_pins.py`:
- Line 108: The condition that skips validation for reusable workflow references
(the check using action.startswith("./") and the string "/.github/workflows/" in
the variable action) must be removed so jobs.<id>.uses entries are subject to
the same 40-char SHA pin, inline "# vX.Y.Z" comment, and SHA↔tag consistency
checks; update the logic in the function that iterates over action (in
dev/check_action_pins.py) to treat any non-local "./" reference the same
regardless of "/.github/workflows/" and ensure the existing validators for SHA
pinning and version comment are invoked for those entries, then update the
reusable-workflow uses lines in the repo to include the required "# vX.Y.Z"
comment matching the pinned SHA.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: dc4d3ce8-1e67-4475-9484-958f41ba7a69
📒 Files selected for processing (3)
.github/policy.rego.github/workflows/operator-integration-tests.ymldev/check_action_pins.py
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/operator-integration-tests.yml
Signed-off-by: kramaranya <kramaranya15@gmail.com>
kramaranya
force-pushed
the
operator-integration-tests
branch
from
83b9729 to
4c2a874
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
dev/check_actions.py (2)
207-215: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPath validation may produce false positives for future files.
_check_paths()requires patterns to match currently tracked files. This breaks for:
- Patterns protecting against future file additions (
paths-ignore)- Files not yet committed
- Patterns for files in
.gitignoreConsider whether
paths-ignoreshould be validated differently, or if a warning (not error) is more appropriate.Example: separate handling for ignore patterns
def _check_paths() -> Iterator[str]: files = _list_tracked_files() for path in sorted(_iter_workflow_files()): for event, key, pattern in _iter_path_patterns(path): + # Skip validation for ignore patterns (they protect against future files) + if key == "paths-ignore": + continue if not _pattern_matches(pattern, files): yield ( f"{path}: [on.{event}.{key}] pattern {pattern!r} does not" " match any tracked file" )🤖 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 `@dev/check_actions.py` around lines 207 - 215, The _check_paths() function treats all patterns equally, but paths-ignore patterns are meant to protect against future files and should not be validated against currently tracked files. Modify the function to differentiate between regular path patterns and paths-ignore patterns by checking the key returned from _iter_path_patterns(). For paths-ignore patterns, either skip the validation entirely or emit a warning instead of an error yield, since these patterns are designed to prevent triggers on files that may be added in the future or are intentionally ignored in git.
143-173: 📐 Maintainability & Code Quality | 🔵 TrivialAdd support for
?and[]glob metacharacters in path filter conversion.
_LITERAL_CHARSrejects?and[], which GitHub Actions minimatch explicitly supports. The code will raiseValueErrorfor workflows using these valid patterns. Note: brace expansion ({}) is intentionally unsupported by GitHub Actions, so rejecting it is correct.🤖 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 `@dev/check_actions.py` around lines 143 - 173, The _glob_to_regex function needs to support two additional glob metacharacters that are valid in GitHub Actions minimatch patterns but are currently unsupported. Add a new condition to handle the `?` character which should match any single character except forward slash (convert to `[^/]` in regex), and add another condition to handle bracket expressions `[]` which should match any character within the brackets by parsing from the opening bracket to the closing bracket, escaping special regex characters within, and appending the resulting character class. Insert these new conditions in the while loop before the _LITERAL_CHARS.match check to give them proper precedence over literal character matching.
🤖 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.
Nitpick comments:
In `@dev/check_actions.py`:
- Around line 207-215: The _check_paths() function treats all patterns equally,
but paths-ignore patterns are meant to protect against future files and should
not be validated against currently tracked files. Modify the function to
differentiate between regular path patterns and paths-ignore patterns by
checking the key returned from _iter_path_patterns(). For paths-ignore patterns,
either skip the validation entirely or emit a warning instead of an error yield,
since these patterns are designed to prevent triggers on files that may be added
in the future or are intentionally ignored in git.
- Around line 143-173: The _glob_to_regex function needs to support two
additional glob metacharacters that are valid in GitHub Actions minimatch
patterns but are currently unsupported. Add a new condition to handle the `?`
character which should match any single character except forward slash (convert
to `[^/]` in regex), and add another condition to handle bracket expressions
`[]` which should match any character within the brackets by parsing from the
opening bracket to the closing bracket, escaping special regex characters
within, and appending the resulting character class. Insert these new conditions
in the while loop before the _LITERAL_CHARS.match check to give them proper
precedence over literal character matching.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 4a6ae3e5-e609-4015-9b25-6509ef1c33ee
📒 Files selected for processing (3)
.github/policy.rego.github/workflows/operator-integration-tests.ymldev/check_actions.py
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/operator-integration-tests.yml
- .github/policy.rego
2 participants
Summary
Adds a caller workflow that triggers the MLflow operator's integration tests
Depends on: opendatahub-io/mlflow-operator#123
RHOAIENG-62014
Upstream / Downstream Impact
opendatahub-io/mlflowmlflow/mlflowIf relevant, add any upstream issue or follow-up link here:
Testing
Summary by CodeRabbit