Kampi/fix changelog check#36
Conversation
- Add Copilot instructions - Documentation updated - Component release workflow optimized Signed-off-by: Daniel Kampert <DanielKampert@kampis-elektroecke.de>
Closes: Issue #29 Signed-of-by: Daniel Kampert <DanielKampert@kampis-elektroecke.de>
- Rework the release pipeline Closes: #32 Signed-off-by: Daniel Kampert <DanielKampert@kampis-elektroecke.de>
- Fix Check-Changelog CI/CD failure for the Unrelease section in CHANGELOG.md Closes #35 Signed-off-by: Daniel Kampert <DanielKampert@kampis-elektroecke.de>
Kampi
added
bug
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds comprehensive contributor guidelines documentation in Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 PR refactors the CI/CD workflows to fix the changelog check and improve the release process. The changes update paths from hardware/CHANGELOG.md to CHANGELOG.md, add version handling improvements, and include comprehensive GitHub Copilot coding guidelines.
Changes:
- Fixed changelog check workflow to reference correct CHANGELOG.md path and improve validation behavior
- Enhanced release workflow with better version extraction, CMakeLists.txt updates, and GitHub CLI-based release creation
- Simplified documentation workflow by removing nightly release builds and focusing on GitHub Pages deployment
- Added comprehensive GitHub Copilot instructions document with project-specific coding standards
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Added ESP Registry badge for component visibility |
| .github/workflows/release.yaml | Major refactoring: added version suffix removal, improved CMakeLists.txt updates, switched to GitHub CLI for releases |
| .github/workflows/documentation.yaml | Simplified to remove nightly builds and focus on GitHub Pages deployment only |
| .github/workflows/changelog-check.yaml | Updated changelog path and relaxed validation to allow empty/incomplete sections during development |
| .github/workflows/astyle.yaml | Removed unused environment variable |
| .github/copilot-instructions.md | New comprehensive coding guidelines document for the project |
| [](https://www.gnu.org/licenses/gpl-3.0) | ||
| [](https://github.com/espressif/esp-idf) | ||
| [](https://kampi.github.io/ESP32-Lepton/) | ||
| [](https://components.espressif.com/components/kampi/esp32-lepton/badge.svg) |
There was a problem hiding this comment.
The ESP Registry badge URL is incorrect. The badge should link to the component page URL, not to the badge image URL itself. Currently, clicking the badge will display the SVG image instead of navigating to the component page. The href should be "https://components.espressif.com/components/kampi/esp32-lepton" (without the trailing "/badge.svg").
| [](https://components.espressif.com/components/kampi/esp32-lepton/badge.svg) | |
| [](https://components.espressif.com/components/kampi/esp32-lepton) |
| # Remove _Dev suffix for the release version | ||
| RELEASE_VERSION=$(echo "$VERSION" | sed 's/_Dev$//') |
There was a problem hiding this comment.
The VERSION extraction logic assumes the version format in idf_component.yml is always 'version: "X.X.X_Dev"', but according to the current idf_component.yml file (version: "0.0.2"), there is no "_Dev" suffix. This means the sed command 's/_Dev$//' will have no effect on versions that don't have this suffix. Consider adding a check to verify if the "_Dev" suffix exists before attempting to remove it, or document the expected version format in idf_component.yml for development versions.
| if ! grep -q "set(${REPO_NAME}_LIB_MAJOR" CMakeLists.txt; then | ||
| # Add version variables before register_component() | ||
| sed -i "/register_component()/i # Library version\nset(${REPO_NAME}_LIB_MAJOR $MAJOR)\nset(${REPO_NAME}_LIB_MINOR $MINOR)\nset(${REPO_NAME}_LIB_BUILD $BUILD)\n" CMakeLists.txt | ||
| else | ||
| # Update existing version variables | ||
| sed -i -E "s/set\(${REPO_NAME}_LIB_MAJOR [0-9]+\)/set(${REPO_NAME}_LIB_MAJOR $MAJOR)/" CMakeLists.txt | ||
| sed -i -E "s/set\(${REPO_NAME}_LIB_MINOR [0-9]+\)/set(${REPO_NAME}_LIB_MINOR $MINOR)/" CMakeLists.txt | ||
| sed -i -E "s/set\(${REPO_NAME}_LIB_BUILD [0-9]+\)/set(${REPO_NAME}_LIB_BUILD $BUILD)/" CMakeLists.txt | ||
| fi |
There was a problem hiding this comment.
The CMakeLists.txt update logic assumes version variables don't exist and will add them before "register_component()", but looking at the current CMakeLists.txt file (lines 30-33), the version variables are referenced AFTER "register_component()" in target_compile_definitions. This means the new logic will create duplicate version variable references. The check at line 100 should verify if the version variables exist anywhere in the file, not just check if they need to be added before register_component(). Additionally, if they exist after register_component(), they should be updated in place rather than adding new ones before it.
| - name: Wait for tag to be available | ||
| run: | | ||
| echo "Waiting for tag to be fully available..." | ||
| sleep 5 | ||
| git fetch --tags | ||
| git tag -l "$VERSION" | ||
|
|
There was a problem hiding this comment.
Using a fixed 5-second sleep to wait for tag availability is unreliable and inefficient. In most cases, the tag should be immediately available after pushing, making the wait unnecessary. In edge cases where there might be replication delays, 5 seconds might not be enough. Consider implementing a retry loop with exponential backoff, or remove this step entirely since "git fetch --tags" should handle any synchronization needed.
| - name: Wait for tag to be available | |
| run: | | |
| echo "Waiting for tag to be fully available..." | |
| sleep 5 | |
| git fetch --tags | |
| git tag -l "$VERSION" |
| run: | | ||
| # Extract the section for the current version from CHANGELOG.md | ||
| awk -v version="$VERSION" ' | ||
| /^## \['"$VERSION"'\]/ { found=1; next } |
There was a problem hiding this comment.
The awk script for extracting release notes is inconsistent in how it uses the version variable. Line 149 correctly passes VERSION to awk using '-v version="$VERSION"', but line 150 uses shell variable expansion '/^## ['"$VERSION"']/' instead of using the awk variable. This mixing of approaches works but is confusing and harder to maintain. Consider using the awk variable consistently: change line 150 to '/^## [/ && $0 ~ "\[" version "\]"' to match the version using the awk variable that was already defined.
| /^## \['"$VERSION"'\]/ { found=1; next } | |
| /^## \[/ && $0 ~ "\\[" version "\\]" { found=1; next } |
| gh release create "$VERSION" \ | ||
| --title "Release $VERSION" \ | ||
| --notes-file RELEASE_NOTES.md \ | ||
| --target main No newline at end of file |
There was a problem hiding this comment.
The GitHub release creation specifies '--target main' (line 171) which hardcodes the target branch to 'main'. However, this workflow runs on both 'main' and 'master' branches according to the trigger (lines 5-7). If a release is created from the 'master' branch, the '--target main' will incorrectly reference the 'main' branch. Consider using '--target ${{ github.ref_name }}' to dynamically reference the branch that triggered the workflow.
| --target main | |
| --target ${{ github.ref_name }} |
| if git rev-parse "$VERSION" >/dev/null 2>&1; then | ||
| echo "Tag $VERSION already exists, skipping tag creation" | ||
| else | ||
| echo "## Release $VERSION" >> $GITHUB_ENV | ||
| echo "" >> $GITHUB_ENV | ||
| echo "See [CHANGELOG.md](CHANGELOG.md) for details." >> $GITHUB_ENV | ||
| git tag -a "$VERSION" -m "Release $VERSION" | ||
| git push origin "$VERSION" | ||
| echo "Tag $VERSION created and pushed" | ||
| fi |
There was a problem hiding this comment.
The git tag check uses "git rev-parse" which checks if a ref exists (tag, branch, or commit), but doesn't specifically verify if it's a tag. This could cause issues if a branch with the same name as the version exists. Consider using "git tag -l "$VERSION"" instead to specifically check for tag existence, which is more explicit and safer.
| if [ "$FOUND_SECTIONS" = false ]; then | ||
| echo "Error: No valid sections found in [Unreleased]" | ||
| echo "Warning: No valid sections found in [Unreleased], but content exists" | ||
| echo "Expected sections: Added, Changed, Fixed, Removed" | ||
| exit 1 | ||
| echo "This is allowed during early development" | ||
| exit 0 |
There was a problem hiding this comment.
The error message was changed from an error to a success message, but the behavior is inconsistent with the validation intent. When the [Unreleased] section is empty (line 44), the script exits with code 0 (success) saying "allowed during development". However, when no valid sections are found but content exists (lines 63-66), it also exits with code 0 despite potentially having incorrectly formatted content. This could allow malformed changelog entries to pass validation. Consider whether this should remain a warning with exit 0, or if it should be a failure to enforce proper formatting.
| # Extract repository name and convert to uppercase with underscores | ||
| REPO_NAME=$(echo "${{ github.repository }}" | cut -d'/' -f2 | tr '[:lower:]' '[:upper:]' | tr '-' '_') |
There was a problem hiding this comment.
The script extracts the repository name and converts it to uppercase with underscores for variable naming (line 97). However, if the repository name contains characters other than lowercase letters, numbers, and hyphens, the conversion might produce invalid or unexpected variable names. Consider adding validation or sanitization to ensure the resulting variable name is valid for CMake (only alphanumeric and underscore characters).
| # Extract repository name and convert to uppercase with underscores | |
| REPO_NAME=$(echo "${{ github.repository }}" | cut -d'/' -f2 | tr '[:lower:]' '[:upper:]' | tr '-' '_') | |
| # Extract repository name, sanitize for CMake (alnum and underscore only), and convert to uppercase | |
| REPO_NAME=$(echo "${{ github.repository }}" | cut -d'/' -f2 | sed 's/[^A-Za-z0-9]/_/g' | tr '[:lower:]' '[:upper:]') |
| # Add version variables before register_component() | ||
| sed -i "/register_component()/i # Library version\nset(${REPO_NAME}_LIB_MAJOR $MAJOR)\nset(${REPO_NAME}_LIB_MINOR $MINOR)\nset(${REPO_NAME}_LIB_BUILD $BUILD)\n" CMakeLists.txt |
There was a problem hiding this comment.
The sed command to insert version variables before 'register_component()' uses '/register_component()/i' which will insert before any line containing 'register_component()'. However, looking at the CMakeLists.txt file, line 27 contains just 'register_component()' with no arguments. The insertion command should work, but the newlines in the sed command need careful handling. The command uses '\n' to insert newlines, but in the actual execution context, these may not render as expected. Consider using multiple echo statements or printf to ensure proper formatting of the inserted lines.
| # Add version variables before register_component() | |
| sed -i "/register_component()/i # Library version\nset(${REPO_NAME}_LIB_MAJOR $MAJOR)\nset(${REPO_NAME}_LIB_MINOR $MINOR)\nset(${REPO_NAME}_LIB_BUILD $BUILD)\n" CMakeLists.txt | |
| # Build version block with proper newlines | |
| VERSION_BLOCK="# Library version | |
| set(${REPO_NAME}_LIB_MAJOR $MAJOR) | |
| set(${REPO_NAME}_LIB_MINOR $MINOR) | |
| set(${REPO_NAME}_LIB_BUILD $BUILD) | |
| " | |
| # Insert version block before the first occurrence of register_component() | |
| tmp_file=$(mktemp) | |
| awk -v block="$VERSION_BLOCK" ' | |
| /register_component\(\)/ && !done { | |
| print block | |
| done=1 | |
| } | |
| { print } | |
| ' CMakeLists.txt > "$tmp_file" | |
| mv "$tmp_file" CMakeLists.txt |
Summary by CodeRabbit
Documentation
Chores