Issue #29#30
Conversation
- Add Copilot instructions - Documentation updated - Component release workflow optimized Signed-off-by: Daniel Kampert <DanielKampert@kampis-elektroecke.de>
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThis PR adds comprehensive development guidelines documentation and refactors GitHub Actions workflows. Changes include: adding copilot-instructions.md with project standards, removing hardcoded branch environment variables from multiple workflows, migrating CHANGELOG file path from hardware directory to root, simplifying documentation deployment workflow by removing release artifact generation, and enhancing release workflow version extraction with multi-format support and fallback logic. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 |
There was a problem hiding this comment.
Pull request overview
This pull request addresses Issue #29 by improving GitHub Actions workflows and adding comprehensive development guidelines. The changes enhance the release automation, simplify documentation deployment, and establish coding standards through new Copilot instructions.
Changes:
- Enhanced version extraction logic in the release workflow to support multiple merge commit formats
- Simplified documentation workflow by removing nightly release artifacts and focusing solely on GitHub Pages deployment
- Updated changelog validation to check the root-level CHANGELOG.md instead of hardware/CHANGELOG.md
- Added comprehensive GitHub Copilot instructions documenting code style, naming conventions, and development practices
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| .github/workflows/release.yaml | Enhanced version extraction with multiple merge format support, added automated tag creation step, and improved error reporting |
| .github/workflows/documentation.yaml | Simplified deployment by removing nightly release logic, removed unnecessary environment variables, and restricted deployment to push events only |
| .github/workflows/changelog-check.yaml | Updated changelog file path from hardware/CHANGELOG.md to root-level CHANGELOG.md, removed paths filters |
| .github/workflows/astyle.yaml | Removed unused master_branch environment variable |
| .github/copilot-instructions.md | Added comprehensive development guidelines covering code style, naming conventions, documentation requirements, and ESP-IDF best practices |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| deploy: | ||
| if: github.event_name != 'pull_request' && (github.ref == 'refs/heads/main' || github.ref == 'refs/heads/master' || contains(github.ref, 'refs/tags/')) | ||
| if: github.event_name == 'push' && (github.ref == 'refs/heads/main' || github.ref == 'refs/heads/master') |
There was a problem hiding this comment.
The deployment condition was changed from github.event_name != 'pull_request' && (github.ref == 'refs/heads/main' || github.ref == 'refs/heads/master' || contains(github.ref, 'refs/tags/')) to github.event_name == 'push' && (github.ref == 'refs/heads/main' || github.ref == 'refs/heads/master').
This removes the ability to deploy documentation when tags are pushed. If the repository relies on tag-based documentation deployment (e.g., versioned documentation for releases), this functionality has been removed. Verify that tag-based documentation deployment is no longer needed, or restore the tag condition if it's still required.
| if: github.event_name == 'push' && (github.ref == 'refs/heads/main' || github.ref == 'refs/heads/master') | |
| if: github.event_name != 'pull_request' && (github.ref == 'refs/heads/main' || github.ref == 'refs/heads/master' || contains(github.ref, 'refs/tags/')) |
| # Get the previous release tag (without v prefix) | ||
| PREVIOUS_TAG=$(git tag --sort=-v:refname | grep -E '^[0-9]+\.[0-9]+\.[0-9]+' | head -n 1 || true) |
There was a problem hiding this comment.
The PREVIOUS_TAG variable is extracted twice using the same command: once in the "Update CHANGELOG.md" step (line 74) and again in the "Generate release notes with diff" step (line 124). This is redundant and could lead to inconsistency if the git state changes between steps (though unlikely in a workflow context).
Consider extracting PREVIOUS_TAG once in an earlier step and storing it in GITHUB_ENV, then referencing it in both steps. This would improve maintainability and ensure consistency.
| # Get the previous release tag (without v prefix) | |
| PREVIOUS_TAG=$(git tag --sort=-v:refname | grep -E '^[0-9]+\.[0-9]+\.[0-9]+' | head -n 1 || true) | |
| # Use the previously computed release tag (without v prefix) | |
| PREVIOUS_TAG="${{ env.PREVIOUS_TAG }}" |
| sed -i -E "s/set\([A-Z_]*VERSION[A-Z_]* [^)]*\)/set(\0 $VERSION)/g" CMakeLists.txt || true | ||
| sed -i -E "s/set\([A-Z_]*LIB_VERSION[A-Z_]* [^)]*\)/set(\0 $VERSION)/g" CMakeLists.txt || true |
There was a problem hiding this comment.
The sed replacement logic for CMakeLists.txt version updates appears to be incorrect. The pattern set(\0 $VERSION) will insert the VERSION variable inside the parentheses instead of replacing the actual value.
Looking at the actual CMakeLists.txt file (lines 30-34), it uses ESP32_LEPTON_LIB_MAJOR, ESP32_LEPTON_LIB_MINOR, and ESP32_LEPTON_LIB_BUILD as variables that reference undefined variables (there are no set() statements defining these values). This sed command will not correctly update version values.
Consider either:
- Adding explicit
set()statements for these version components at the top of CMakeLists.txt and updating those - Passing version components as CMake cache variables during build
- Using a more targeted sed pattern that correctly replaces the value portion of the set() command
| sed -i -E "s/set\([A-Z_]*VERSION[A-Z_]* [^)]*\)/set(\0 $VERSION)/g" CMakeLists.txt || true | |
| sed -i -E "s/set\([A-Z_]*LIB_VERSION[A-Z_]* [^)]*\)/set(\0 $VERSION)/g" CMakeLists.txt || true | |
| sed -i -E "s/(set\([A-Z_]*VERSION[A-Z_]* )[^)]*\)/\1$VERSION)/g" CMakeLists.txt || true | |
| sed -i -E "s/(set\([A-Z_]*LIB_VERSION[A-Z_]* )[^)]*\)/\1$VERSION)/g" CMakeLists.txt || true |
| @@ -5,19 +5,15 @@ on: | |||
| branches: | |||
| - main | |||
| - master | |||
There was a problem hiding this comment.
The push trigger now builds documentation for all changes to main/master branches, not just documentation-related changes. This means the documentation build will run on every commit to these branches, including changes to source code, workflows, or other non-documentation files.
This could result in unnecessary CI/CD resource usage. The paths filter was removed from the push trigger but kept for pull_request. Consider whether this is intentional. If documentation should only rebuild when docs-related files change, restore the paths filter:
push:
branches:
- main
- master
paths:
- 'docs/**'
- '.github/workflows/documentation.yaml'| - master | |
| - master | |
| paths: | |
| - 'docs/**' | |
| - '.github/workflows/documentation.yaml' |
| * You should have received a copy of the GNU General Public License | ||
| * along with this program. If not, see <https://www.gnu.org/licenses/>. | ||
| * | ||
| * Errors and commissions should be reported to DanielKampert@kampis-elektroecke.de |
There was a problem hiding this comment.
The word "commissions" should be "omissions" in this context. The phrase "Errors and commissions" is incorrect - the standard phrase is "Errors and omissions" which refers to mistakes (errors) and things left out (omissions).
| * Errors and commissions should be reported to DanielKampert@kampis-elektroecke.de | |
| * Errors and omissions should be reported to DanielKampert@kampis-elektroecke.de |
|
|
||
| deploy: | ||
| if: github.event_name != 'pull_request' && (github.ref == 'refs/heads/main' || github.ref == 'refs/heads/master' || contains(github.ref, 'refs/tags/')) | ||
| if: github.event_name == 'push' && (github.ref == 'refs/heads/main' || github.ref == 'refs/heads/master') |
There was a problem hiding this comment.
The workflow_dispatch trigger allows manual workflow runs, but the deploy step will only execute when github.event_name == 'push'. This means manual workflow runs via workflow_dispatch will build the documentation but won't deploy it to GitHub Pages.
This could be confusing for users who manually trigger the workflow expecting a deployment. Consider either:
- Adding 'workflow_dispatch' to the deploy condition:
if: (github.event_name == 'push' || github.event_name == 'workflow_dispatch') && ... - Documenting that manual runs don't deploy
- Removing workflow_dispatch if manual deployment is not needed
| if: github.event_name == 'push' && (github.ref == 'refs/heads/main' || github.ref == 'refs/heads/master') | |
| if: (github.event_name == 'push' || github.event_name == 'workflow_dispatch') && (github.ref == 'refs/heads/main' || github.ref == 'refs/heads/master') |
| - name: Create and push tag | ||
| run: | | ||
| git config user.name "github-actions[bot]" | ||
| git config user.email "41898282+github-actions[bot]@users.noreply.github.com" | ||
| git tag -a "${{ env.VERSION }}" -m "Release ${{ env.VERSION }}" | ||
| git push origin "${{ env.VERSION }}" |
There was a problem hiding this comment.
The "Create and push tag" step is positioned after "Commit and push changes" but before "Generate release notes". This creates a potential issue: the tag will be created and pushed before the GitHub Release is created. If the GitHub Release creation fails, the tag will remain in the repository without an associated release.
Additionally, the tag is created after the version update commit, which means the tag will not point to the commit that contains the version updates. The workflow should either:
- Move this step after the GitHub Release is successfully created, or
- Ensure proper error handling and rollback if subsequent steps fail
| - master | ||
| push: | ||
| branches: | ||
| - main | ||
| - master | ||
| - dev |
There was a problem hiding this comment.
The paths filter was removed from both push and pull_request triggers, which means the changelog validation will now run on every commit and pull request, regardless of whether the CHANGELOG.md file was modified.
This results in unnecessary CI/CD runs. The workflow will validate the CHANGELOG.md file even when changes don't affect it, wasting resources and increasing build times. Consider restoring the paths filter to only trigger when CHANGELOG.md is modified:
paths:
- 'CHANGELOG.md'| - master | |
| push: | |
| branches: | |
| - main | |
| - master | |
| - dev | |
| - master | |
| paths: | |
| - 'CHANGELOG.md' | |
| push: | |
| branches: | |
| - main | |
| - master | |
| - dev | |
| paths: | |
| - 'CHANGELOG.md' |
| # Try GitHub pull request merge format: "Merge pull request #123 from user/branch" | ||
| BRANCH_NAME=$(echo "$COMMIT_MSG" | sed -n "s/.*[Mm]erge pull request.*from [^/]*\/\([^ ]*\).*/\1/p" | head -n 1) |
There was a problem hiding this comment.
In the GitHub PR merge format extraction, the regex pattern may not correctly handle branch names that contain spaces or end with newlines. The pattern from [^/]*\/\([^ ]*\) stops at the first space, which could truncate branch names if they contain spaces (though spaces in branch names are uncommon).
More importantly, line 43 extracts the branch name from PR merge messages, but this assumes the PR was merged using the "Merge commit" strategy. GitHub also supports "Squash and merge" and "Rebase and merge", which produce different commit message formats that won't match this pattern.
Consider documenting the required merge strategy or adding support for additional merge commit message formats.
| # Try GitHub pull request merge format: "Merge pull request #123 from user/branch" | |
| BRANCH_NAME=$(echo "$COMMIT_MSG" | sed -n "s/.*[Mm]erge pull request.*from [^/]*\/\([^ ]*\).*/\1/p" | head -n 1) | |
| # Try GitHub pull request merge format (merge commit strategy): | |
| # "Merge pull request #123 from user/branch-name" | |
| # For squash/rebase merges, no "from user/branch" is present; in that case, | |
| # BRANCH_NAME will remain empty and we fall back to extracting VERSION | |
| # directly from the commit message below. | |
| BRANCH_NAME=$(echo "$COMMIT_MSG" | sed -n "s/.*[Mm]erge pull request.*from [^/]*\/\(.*\).*/\1/p" | head -n 1) |
Summary by CodeRabbit
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.