Rework to prek + gh-actions v2 #1002
Conversation
CoMPaTech
added
enhancement
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughReplace pre-commit with prek across CI workflows and scripts, swap node-based markdownlint for pymarkdown with a .pymarkdown config disabling MD013, bump workflow CACHE_VERSION values, update prepare action to v2, and remove PRE_COMMIT_HOME env var. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer (push/PR)
participant GH as GitHub Actions
participant Prep as prepare-python-and-code@v2
participant Runner as Job Runner (VM)
participant Prek as prek (tool)
participant Scripts as scripts/core-testing.sh
Dev->>GH: push / open PR
GH->>Prep: checkout & setup python
Prep-->>Runner: python venv prepared
Runner->>Prek: ensure prek installed (requirements_commit.txt)
Runner->>Prek: prek install (hook setup)
Runner->>Prek: prek run --all-files (pymarkdown, other hooks)
Runner->>Scripts: run scripts/core-testing.sh (invokes prek as needed)
Scripts->>Prek: prek run additional checks
Prek-->>Runner: exit status
Runner-->>GH: job result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learnings📚 Learning: 2025-01-25T20:42:24.862ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (4)
✏️ Tip: You can disable this entire section by setting 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
scripts/core-testing.sh (1)
320-326: Inconsistent usage:pre-commitshould beprek.Line 325 still uses
pre-commitinstead ofprekin theCOMMIT_CHECKblock. This is inconsistent with the migration to prek elsewhere in the script and could fail ifpre-commitis not installed.Proposed fix
if [ -z "${GITHUB_ACTIONS}" ] && [ -n "${COMMIT_CHECK}" ] ; then cd "${coredir}" || exit echo "" echo -e "${CINFO}Core PR pre-commit check ...${CNORM}" echo "" - git add -A ; pre-commit + git add -A ; prek fi.github/workflows/core_next.yml (1)
51-59: Remove theprecommit-homeparameter or define thePRE_COMMIT_HOMEenvironment variable.Line 59 references
${{ env.PRE_COMMIT_HOME }}but this environment variable is not defined in theenvblock. It is also undefined in thetest.ymlworkflow, which has the same reference. This will pass an empty string to theprecommit-homeparameter, which may cause the action to fail or behave unexpectedly..github/workflows/test.yml (2)
85-94: Undefined environment variable referenced.Line 93 references
${{ env.PRE_COMMIT_HOME }}, butPRE_COMMIT_HOMEis not defined in theenvblock (lines 6-9). This will resolve to an empty string. Additionally, other jobs using the same action (lines 106-113, 137-144, 178-185) don't pass theprecommit-homeparameter at all, creating an inconsistency.Either:
- Remove the
precommit-homeparameter from this step (matching other jobs), or- Define
PRE_COMMIT_HOMEin the env block if it's still needed for prekSuggested fix (option 1: remove the parameter)
- name: Prepare code checkout and python/prek/pre-commit setup id: cache-reuse uses: plugwise/gh-actions/prepare-python-and-code@v1 with: cache-key: ${{ needs.cache.outputs.cache-key }} fail-on-miss: false # First time create cache (if not already exists) python-version: ${{ needs.cache.outputs.python-version }} venv-dir: ${{ env.VENV }} - precommit-home: ${{ env.PRE_COMMIT_HOME }} clone-core: "true"
218-222: Pre-existing bug: Incorrect job reference.Line 221 references
needs.ha-core-release.outputs.release_failed, but the job is namedha-core-testing(line 123). This will always return an empty/undefined value, causing the release failure check to never trigger correctly.Suggested fix
- RELEASE_TESTS_FAILED="${{ needs.ha-core-release.outputs.release_failed }}" + RELEASE_TESTS_FAILED="${{ needs.ha-core-testing.outputs.release_failed }}"
🤖 Fix all issues with AI agents
In @CHANGELOG.md:
- Around line 8-10: Update the CHANGELOG entry that currently reads "Replace
node-based markdownlint with pythonesk library" to use a clearer term; replace
"pythonesk library" with either "Python-based library" or the explicit package
name (e.g., "pymarkdown") if that is the actual tool used, so the entry "Replace
node-based markdownlint with pythonesk library" becomes unambiguous.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.github/workflows/core_next.yml.github/workflows/test.yml.markdownlint.yaml.pre-commit-config.yaml.pymarkdownCHANGELOG.mdscripts/core-testing.sh
💤 Files with no reviewable changes (1)
- .markdownlint.yaml
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: CoMPaTech
Repo: plugwise/plugwise-beta PR: 0
File: :0-0
Timestamp: 2025-01-25T20:42:24.862Z
Learning: The linting rules in plugwise-beta have been updated to use TC001/TC002/TC003 instead of TCH001/TCH002/TCH003 for type-checking related rules.
Learnt from: CoMPaTech
Repo: plugwise/plugwise-beta PR: 789
File: .pre-commit-config.yaml:3-3
Timestamp: 2024-12-08T11:23:47.692Z
Learning: CoMPaTech considers Python 3.13 stable and prefers to use it in the project.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Prepare
- GitHub Check: Prepare
- GitHub Check: Prepare
- GitHub Check: Prepare
- GitHub Check: Prepare
🔇 Additional comments (8)
.pymarkdown (1)
1-1: LGTM!The configuration correctly disables the MD013 (line length) rule for pymarkdown. This is a reasonable choice as line length rules often conflict with code blocks, URLs, and tables in Markdown files.
.github/workflows/core_next.yml (1)
6-10: LGTM on cache version bump.Incrementing the cache version is appropriate when changing the tooling from pre-commit to prek.
scripts/core-testing.sh (2)
98-101: LGTM!The migration from
pre-committoprekfor installation and hook setup is correctly implemented.
279-280: LGTM!The pymarkdown integration via prek is correctly configured for the quality check stage.
.pre-commit-config.yaml (1)
119-123: Hook configuration is correct and uses the latest stable version.The pymarkdown hook at v0.9.34 is properly configured and uses the most recent release. This aligns with the
.pymarkdownconfiguration file added in this PR..github/workflows/test.yml (3)
6-9: LGTM: Cache version bump is appropriate.Incrementing
CACHE_VERSIONto 3 aligns with the tooling change from pre-commit to prek, ensuring stale caches won't cause issues.
97-120: LGTM: Consistent renaming to prek.The job name, step names, and command invocation are all consistently updated to reference prek. The
prek run --all-files --show-diff-on-failureinvocation with theSKIPenvironment variable for local hooks is appropriate.
137-144: LGTM: Step names consistently updated.Step names are updated to reference "prek/pre-commit" matching the other jobs, and these steps correctly omit the
precommit-homeparameter (further highlighting the inconsistency in thepreparejob noted above).Also applies to: 178-185
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/test.yml (1)
220-220: Bug: Incorrect job reference will cause undefined output.This line references
needs.ha-core-release.outputs.release_failed, but the job is namedha-core-testing(Line 122), notha-core-release. This will result inRELEASE_TESTS_FAILEDbeing empty or undefined.Proposed fix
- RELEASE_TESTS_FAILED="${{ needs.ha-core-release.outputs.release_failed }}" + RELEASE_TESTS_FAILED="${{ needs.ha-core-testing.outputs.release_failed }}"
🧹 Nitpick comments (1)
scripts/core-testing.sh (1)
329-329: Inconsistent: still usespre-commitinstead ofprek.This line still references
pre-commitdirectly while the rest of the script has migrated toprek. For consistency, consider updating this to useprekas well.Suggested fix
- git add -A ; pre-commit + git add -A ; prek run
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/core_next.yml.github/workflows/test.ymlscripts/core-testing.sh
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CoMPaTech
Repo: plugwise/plugwise-beta PR: 0
File: :0-0
Timestamp: 2025-01-25T20:42:24.862Z
Learning: The linting rules in plugwise-beta have been updated to use TC001/TC002/TC003 instead of TCH001/TCH002/TCH003 for type-checking related rules.
Learnt from: CoMPaTech
Repo: plugwise/plugwise-beta PR: 789
File: .pre-commit-config.yaml:3-3
Timestamp: 2024-12-08T11:23:47.692Z
Learning: CoMPaTech considers Python 3.13 stable and prefers to use it in the project.
📚 Learning: 2025-01-25T20:42:24.862Z
Learnt from: CoMPaTech
Repo: plugwise/plugwise-beta PR: 0
File: :0-0
Timestamp: 2025-01-25T20:42:24.862Z
Learning: In the plugwise-beta project, `uv` package manager was removed from core-testing.sh in favor of pip due to issues it was causing.
Applied to files:
scripts/core-testing.sh
🔇 Additional comments (8)
.github/workflows/core_next.yml (2)
8-8: LGTM!Bumping
CACHE_VERSIONto 3 is appropriate to invalidate caches after the tooling migration from pre-commit to prek.
51-51: LGTM!The step name update reflects the addition of prek tooling. Keeping "pre-commit" in the name is reasonable since prek wraps pre-commit functionality.
scripts/core-testing.sh (3)
69-72: LGTM!Good addition of prek presence check and installation within the venv setup, following the same pattern used for uv.
102-105: LGTM!The migration from pre-commit to prek for hook management is correctly implemented.
283-284: LGTM!Good migration from node-based markdownlint to pymarkdownlint, eliminating the Node.js dependency. The
--hook-stage manualflag is appropriate for this type of validation..github/workflows/test.yml (3)
7-7: LGTM!Cache version bump to 3 aligns with the tooling migration and matches
core_next.yml.
85-85: LGTM!Step and job names consistently updated to reflect prek usage, and the command migration from
pre-commit runtoprek runis correct.Also applies to: 98-98, 105-105, 113-117
136-136: LGTM!Step names in the testing jobs consistently updated to reflect the prek migration.
Also applies to: 177-177
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/core_next.yml:
- Around line 51-59: Rename the workflow step name from "Prepare code checkout
and python/prek/pre-commit setup" to "Prepare code checkout and python/prek
setup" to remove the misleading "pre-commit" mention, and open an issue against
the plugwise/gh-actions repo reporting the typo in the action.yml where the
input `clone-core` is incorrectly referenced as `${{ inputs.cone-core }}` so
that `clone-core: "true"` is passed correctly to the action; include the exact
broken reference `${{ inputs.cone-core }}` and suggest changing it to `${{
inputs.clone-core }}` in your issue so maintainers can fix the input mapping.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/core_next.yml.github/workflows/test.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/test.yml
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: CoMPaTech
Repo: plugwise/plugwise-beta PR: 0
File: :0-0
Timestamp: 2025-01-25T20:42:24.862Z
Learning: The linting rules in plugwise-beta have been updated to use TC001/TC002/TC003 instead of TCH001/TCH002/TCH003 for type-checking related rules.
Learnt from: CoMPaTech
Repo: plugwise/plugwise-beta PR: 789
File: .pre-commit-config.yaml:3-3
Timestamp: 2024-12-08T11:23:47.692Z
Learning: CoMPaTech considers Python 3.13 stable and prefers to use it in the project.
🔇 Additional comments (1)
.github/workflows/core_next.yml (1)
6-10: LGTM!Cache version bump is appropriate when migrating tooling from pre-commit to prek, ensuring fresh cache creation without stale artifacts.
There was a problem hiding this comment.
Pull request overview
This PR modernizes the CI/CD tooling by introducing prek (a pre-commit wrapper) and upgrading to GitHub Actions v2, while also replacing the Node.js-based markdownlint with Python-based pymarkdown for better integration with the Python ecosystem.
Changes:
- Replaced
pre-commitwithprekwrapper across scripts and workflows - Switched from
markdownlinttopymarkdownwith new.pymarkdownconfiguration file - Updated GitHub Actions from
plugwise/gh-actions@v1to@v2with adjusted parameters - Bumped cache version from 1/2 to 3 to reflect tooling changes
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/core-testing.sh | Added prek installation logic, replaced pre-commit commands with prek equivalents, updated markdownlint to pymarkdown |
| CHANGELOG.md | Documented the prek and pymarkdown migration as chore items |
| .pymarkdown | New configuration file for pymarkdown disabling MD013 (line length) rule |
| .pre-commit-config.yaml | Replaced markdownlint-cli repository with pymarkdown repository and updated hook configuration |
| .markdownlint.yaml | Removed legacy markdownlint configuration file (no longer needed) |
| .github/workflows/test.yml | Updated cache version to 3, removed PRE_COMMIT_HOME env var, upgraded action version to v2, updated step names to reference prek |
| .github/workflows/core_next.yml | Updated cache version to 3, removed PRE_COMMIT_HOME env var, upgraded action version to v2, updated step names to reference prek |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
.github/workflows/test.yml:62
- The cache key computation should include 'requirements_commit.txt' in the hashFiles() call since this new file affects the environment dependencies. Without it, changes to requirements_commit.txt won't invalidate the cache, potentially causing builds to use outdated cached environments.
run: echo "cache-key=${{ runner.os }}--${{ env.CACHE_VERSION }}-${{ hashFiles('pyproject.toml', 'requirements_test.txt', '.pre-commit-config.yaml', 'ha_pyproject.toml') }}" >> "$GITHUB_OUTPUT"
.github/workflows/core_next.yml:43
- The cache key computation should include 'requirements_commit.txt' in the hashFiles() call since this new file affects the environment dependencies. Without it, changes to requirements_commit.txt won't invalidate the cache, potentially causing builds to use outdated cached environments.
run: echo "cache-key=${{ runner.os }}-${{ env.CACHE_VERSION }}-${{ hashFiles('pyproject.toml', 'requirements_test.txt', '.pre-commit-config.yaml', 'ha_pyproject.toml') }}-dev-branch" >> "$GITHUB_OUTPUT"
scripts/core-testing.sh:327
- This line still uses 'pre-commit' directly instead of 'prek'. It should be changed to 'prek' for consistency with the rest of the migration. Consider using 'prek run' or the appropriate prek command.
git add -A ; pre-commit
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.