Skip to content

fix(ci): check_dependencies - remove check_auth run#23

Merged
BenjaminLangenakenSF merged 3 commits intomainfrom
feat/check-dependencies-cli-api-credentials
Mar 27, 2026
Merged

fix(ci): check_dependencies - remove check_auth run#23
BenjaminLangenakenSF merged 3 commits intomainfrom
feat/check-dependencies-cli-api-credentials

Conversation

@BenjaminLangenakenSF
Copy link
Copy Markdown
Contributor

@BenjaminLangenakenSF BenjaminLangenakenSF commented Mar 27, 2026

Summary

The Check dependencies reusable workflow failed during setup because it ran silverfin-cli -V after npm install. That invocation triggers the CLI’s credential check, even though check-dependencies only scans local Liquid test YAML and does not need Silverfin API access for that work.
This PR removes the unnecessary cli.js -V call and keeps setup as install only, before running check-dependencies per handle.

Changes

  • Remove node ./node_modules/silverfin-cli/bin/cli.js -V from the “Setup Node and Silverfin CLI” step.
  • Adjust the header comment to match (local scan only; no check_auth / token refresh in this workflow).

Notes

  • This avoids wiring SF_API_CLIENT_ID / SF_API_SECRET into this workflow solely to satisfy a redundant version check.
  • If silverfin-cli check-dependencies itself enforces credentials at runtime in the future, that would be a separate CLI/product concern; this change only removes the extra step that caused the failure you observed.

silverfin-cli now requires API credentials at startup (including -V).
Caller repos should use secrets: inherit when calling this workflow.

Made-with: Cursor
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 27, 2026

Walkthrough

Removed the dedicated "Setup Node and Silverfin CLI" step from .github/workflows/check_dependencies.yml; moved the npm install https://github.com/silverfin/silverfin-cli.git invocation into the "Run check-dependencies per handle" step’s shell script and updated the workflow comment to clarify the job scans local Liquid Test YAML files only.

Changes

Cohort / File(s) Summary
GitHub Actions Workflow
​.github/workflows/check_dependencies.yml
Deleted the separate setup step that ran a CLI version check; relocated the CLI npm install https://github.com/silverfin/silverfin-cli.git into the per-handle "Run check-dependencies" step script. Updated top comment to state the job scans only local Liquid Test YAML files. No other job conditions, outputs, loop logic, or command invocations were changed.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Title check ⚠️ Warning The PR title references 'check_auth run' removal, but the actual change is about removing a version check and moving CLI installation into the check-dependencies step, not related to auth. Update the title to accurately reflect the main change: e.g., 'fix(ci): remove unnecessary CLI version check from check_dependencies workflow' or 'refactor(ci): inline silverfin-cli install into check-dependencies step'.
✅ Passed checks (2 passed)
Check name Status Explanation
Description check ✅ Passed The description includes summary and changes sections but is missing the 'Type of change' and 'Checklist' sections from the required template.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ 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 feat/check-dependencies-cli-api-credentials

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

@BenjaminLangenakenSF BenjaminLangenakenSF self-assigned this Mar 27, 2026
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.github/workflows/check_dependencies.yml:
- Around line 26-29: The SF_API_CLIENT_ID and SF_API_SECRET are currently set at
job scope (env block) and thus exposed to all steps; move these two secrets from
the job-level env to the specific step-level env entries for the two CLI steps
that actually need them (the steps that run your CLI commands), leaving other
steps (e.g., actions/checkout, actions/github-script) without those secrets;
update the two CLI step definitions to include env: SF_API_CLIENT_ID: ${{
secrets.SF_API_CLIENT_ID }} and SF_API_SECRET: ${{ secrets.SF_API_SECRET }} and
remove them from the job env block.
🪄 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: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 485319f7-0bec-4223-b6a5-09241f2fead4

📥 Commits

Reviewing files that changed from the base of the PR and between d70b17c and fa9f4ce.

📒 Files selected for processing (1)
  • .github/workflows/check_dependencies.yml

Drop the unnecessary  execution from setup and keep only installation before running check-dependencies.

Made-with: Cursor
Remove the separate setup step; npm install must still run before cli.js.

Made-with: Cursor
@BenjaminLangenakenSF BenjaminLangenakenSF changed the title fix(ci): pass API credentials to check-dependencies workflow fix(ci): remove check_auth run Mar 27, 2026
@BenjaminLangenakenSF BenjaminLangenakenSF changed the title fix(ci): remove check_auth run fix(ci): check_dependencies - remove check_auth run Mar 27, 2026
@michieldegezelle michieldegezelle self-requested a review March 27, 2026 10:13
@BenjaminLangenakenSF BenjaminLangenakenSF merged commit ac1badc into main Mar 27, 2026
7 checks passed
@BenjaminLangenakenSF BenjaminLangenakenSF deleted the feat/check-dependencies-cli-api-credentials branch March 27, 2026 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants