Skip to content

Updated BOM tests#415

Open
kozlov721 wants to merge 4 commits into
mainfrom
tests/bom-update
Open

Updated BOM tests#415
kozlov721 wants to merge 4 commits into
mainfrom
tests/bom-update

Conversation

@kozlov721

@kozlov721 kozlov721 commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator

Purpose

Reuses the CI workflow for BOM tests instead of using a separate workflow for it.

Specification

None / not applicable

Dependencies & Potential Impact

None / not applicable

Deployment Plan

None / not applicable

Testing & Validation

None / not applicable

AI Usage

Assisted-by: AGENT_NAME:MODEL_VERSION [TOOL1] [TOOL2]

Submitted code was reviewed by a human: YES/NO

The author is taking the responsibility for the contribution: YES/NO

Summary by CodeRabbit

  • Chores
    • Updated automated checks to support reusable and manually triggered runs with configurable build and training references.
    • Improved dependency selection during CI so validation can use either a released build or a source version, depending on the run.
    • Removed a separate test workflow and consolidated testing into the updated CI setup.

@kozlov721 kozlov721 requested a review from a team as a code owner July 8, 2026 13:01
@kozlov721 kozlov721 requested review from conorsim, klemen1999 and tersekmatija and removed request for a team July 8, 2026 13:01
@github-actions github-actions Bot added the tests Adding or changing tests label Jul 8, 2026
@coderabbitai

coderabbitai Bot commented Jul 8, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The CI workflow (ci.yaml) is parameterized with ml_ref/train_ref inputs and required secrets to support workflow_call and workflow_dispatch triggers. Job checkout steps and luxonis-ml dependency installation logic are updated to use these refs. The separate tests.yaml workflow is removed entirely.

Changes

CI Workflow Consolidation

Layer / File(s) Summary
Workflow inputs and secrets
.github/workflows/ci.yaml
Adds ml_ref/train_ref inputs to workflow_dispatch and workflow_call, plus required secrets for GCP, Codecov, and HubAI.
Checkout ref propagation
.github/workflows/ci.yaml
Updates checkout steps across pre-commit, docs, type-check, config-test, and tests jobs to select Luxonis/luxonis-train at inputs.train_ref with fallback behavior.
Conditional luxonis-ml installation
.github/workflows/ci.yaml
Replaces hardcoded dev/release install branching with logic that installs a released build or a source build pinned to ${ML_REF} depending on inputs.ml_ref and branch name, across type-check, config-test, and tests jobs.
Removal of standalone tests workflow
.github/workflows/tests.yaml
Deletes the separate Run tests workflow, whose functionality is now folded into ci.yaml.

Estimated code review effort: 2 (Simple) | ~15 minutes

Possibly related PRs

  • luxonis/luxonis-train#393: Also modifies .github/workflows/tests.yaml steps, directly related through changes to the same workflow file.

Suggested reviewers: klemen1999, tersekmatija, conorsim

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title is related to the PR’s BOM testing update, though it doesn’t mention the workflow reuse/refactor.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch tests/bom-update

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@kozlov721 kozlov721 changed the base branch from main to fix/luxonis-ml July 8, 2026 13:08

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
.github/workflows/ci.yaml (1)

61-62: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Add persist-credentials: false to all checkout steps.

None of the five checkout steps set persist-credentials: false, so the GITHUB_TOKEN is persisted in .git/config after checkout. This PR introduces cross-repository checkouts (Luxonis/luxonis-train when train_ref is provided), which means the persisted token grants access to the upstream repo — widening the blast radius if any future step uploads the workspace or a subdirectory containing .git.

🔒 Proposed fix — apply to all five checkout steps
       - name: Checkout
         uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0
         with:
           repository: ${{ inputs.train_ref != '' && 'Luxonis/luxonis-train' || github.repository }}
           ref: ${{ inputs.train_ref || github.head_ref || github.ref }}
+          persist-credentials: false

Also applies to: 78-79, 95-96, 137-138, 187-188

🤖 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 @.github/workflows/ci.yaml around lines 61 - 62, Add persist-credentials:
false to every actions/checkout step in this workflow, including the checkout
that uses inputs.train_ref and the other four checkout invocations. The issue is
that checkout currently leaves the GITHUB_TOKEN in .git/config, which is
especially risky for cross-repository fetches; update each checkout
configuration to disable credential persistence consistently across all
occurrences.

Source: Linters/SAST tools

🤖 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 @.github/workflows/ci.yaml:
- Around line 61-62: Add persist-credentials: false to every actions/checkout
step in this workflow, including the checkout that uses inputs.train_ref and the
other four checkout invocations. The issue is that checkout currently leaves the
GITHUB_TOKEN in .git/config, which is especially risky for cross-repository
fetches; update each checkout configuration to disable credential persistence
consistently across all occurrences.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 02496a4e-550a-497b-8dd7-4ad870f45cb1

📥 Commits

Reviewing files that changed from the base of the PR and between 9b463a8 and d40f510.

📒 Files selected for processing (2)
  • .github/workflows/ci.yaml
  • .github/workflows/tests.yaml
💤 Files with no reviewable changes (1)
  • .github/workflows/tests.yaml

@kozlov721 kozlov721 requested a review from dtronmans July 8, 2026 13:25
@codecov

codecov Bot commented Jul 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 61.84%. Comparing base (1e9a20e) to head (dbf4c00).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@               Coverage Diff               @@
##           fix/luxonis-ml     #415   +/-   ##
===============================================
  Coverage           61.84%   61.84%           
===============================================
  Files                 268      268           
  Lines               13106    13106           
===============================================
  Hits                 8105     8105           
  Misses               5001     5001           
Flag Coverage Δ
unit 61.84% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

Base automatically changed from fix/luxonis-ml to main July 9, 2026 08:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tests Adding or changing tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants