fix: sign only idf repo plugins and simplifying the P2 metadata #1368
Conversation
WalkthroughModified CI release workflow's JAR signing process to target only IDF-related plugins and features with stricter file filtering and additional signing parameters. Improved error handling for signing failures and adjusted metadata regeneration step by removing explicit repositoryPath argument with fallback logic. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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
🧹 Nitpick comments (1)
.github/workflows/ci_release.yml (1)
75-76: Minor: Duplicate variable definition across steps.
REPO_DIRis defined in both the signing step (line 75) and the metadata regeneration step (line 128). Consider defining it once as an environment variable at the job level to reduce duplication and improve maintainability.Move to the job-level
envsection:- name: Sign JAR files with jarsigner env: AZURE_TOKEN: ${{ steps.get-token.outputs.token }} CERT_CHAIN: ${{ secrets.AZURE_KEYVAULT_CERTCHAIN }} + REPO_DIR: "releng/com.espressif.idf.update/target/repository" + SIGFILE: "ECLIPSE" run: | - REPO_DIR="releng/com.espressif.idf.update/target/repository" - SIGFILE="ECLIPSE"And remove the redefinition on line 128 in the metadata step.
Also applies to: 128-128
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci_release.yml(1 hunks)
⏰ 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). (3)
- GitHub Check: build_macos
- GitHub Check: build_windows
- GitHub Check: build
🔇 Additional comments (1)
.github/workflows/ci_release.yml (1)
131-137: Clarify: P2 metadata regeneration fallback behavior.The fallback logic silently retries without the explicit
repositoryPathargument if the first command fails. This is reasonable for robustness, but it's unclear whether the failure is expected or signals a real problem. If the path is guaranteed to exist, the first command should always succeed—if it doesn't, the silent fallback might mask underlying issues.Please clarify:
- Is the first command expected to fail in any normal scenario?
- Should we add logging to indicate when/why the fallback is triggered?
- Are there any Maven or Tycho version constraints that would require the fallback?
Alternatively, consider making the fallback explicit with logging:
mvn -f releng/com.espressif.idf.update/pom.xml \ org.eclipse.tycho:tycho-p2-repository-plugin:fix-artifacts-metadata \ -DrepositoryPath="$REPO_DIR" \ -DskipTests=true || { echo "Warning: Metadata regeneration with explicit path failed, retrying without path..." mvn -f releng/com.espressif.idf.update/pom.xml \ org.eclipse.tycho:tycho-p2-repository-plugin:fix-artifacts-metadata \ -DskipTests=true }
| find "$REPO_DIR" -type f -name "*.jar" | while read -r file; do | ||
| if [[ "$file" =~ (plugins|features)/com\.espressif\. ]]; then | ||
| echo "Signing IDF plugin/feature JAR: $file" | ||
|
|
||
| jarsigner \ | ||
| -J-cp -Jjsign-7.4.jar \ | ||
| -J--add-modules -Jjava.sql \ | ||
| -providerClass net.jsign.jca.JsignJcaProvider \ | ||
| -providerArg "${{ secrets.AZURE_KEYVAULT_URI }}" \ | ||
| -keystore NONE \ | ||
| -storetype AZUREKEYVAULT \ | ||
| -storepass "$AZURE_TOKEN" \ | ||
| -sigfile "$SIGFILE" \ | ||
| -digestalg SHA-256 \ | ||
| -tsa http://timestamp.digicert.com \ | ||
| $CERTCHAIN_ARG \ | ||
| -certs \ | ||
| -verbose \ | ||
| "$file" \ | ||
| "${{ secrets.AZURE_KEYVAULT_CERT_NAME }}" | ||
|
|
||
| if [ $? -eq 0 ]; then | ||
| echo "Successfully signed: $file" | ||
| else | ||
| echo "Failed to sign: $file" | ||
| exit 1 | ||
| fi | ||
| else | ||
| echo "Failed to sign: $file" | ||
| exit 1 | ||
| echo "Skipping non-IDF JAR: $file" | ||
| fi | ||
| done |
There was a problem hiding this comment.
Critical: Error handling broken in piped loop—signing failures won't fail the workflow.
The find ... | while read pattern creates a subshell. The exit 1 on line 106 exits the subshell, not the parent workflow step, so signing failures silently succeed. Additionally, $? -eq 0 on line 102 evaluates the exit status of the preceding echo command (due to piping), not the jarsigner command.
Restructure the loop to properly propagate errors:
- find "$REPO_DIR" -type f -name "*.jar" | while read -r file; do
- if [[ "$file" =~ (plugins|features)/com\.espressif\. ]]; then
- echo "Signing IDF plugin/feature JAR: $file"
-
- jarsigner \
- -J-cp -Jjsign-7.4.jar \
- -J--add-modules -Jjava.sql \
- -providerClass net.jsign.jca.JsignJcaProvider \
- -providerArg "${{ secrets.AZURE_KEYVAULT_URI }}" \
- -keystore NONE \
- -storetype AZUREKEYVAULT \
- -storepass "$AZURE_TOKEN" \
- -sigfile "$SIGFILE" \
- -digestalg SHA-256 \
- -tsa http://timestamp.digicert.com \
- $CERTCHAIN_ARG \
- -certs \
- -verbose \
- "$file" \
- "${{ secrets.AZURE_KEYVAULT_CERT_NAME }}"
-
- if [ $? -eq 0 ]; then
- echo "Successfully signed: $file"
- else
- echo "Failed to sign: $file"
- exit 1
- fi
- else
- echo "Skipping non-IDF JAR: $file"
- fi
- done
+ set -e
+ while IFS= read -r file; do
+ if [[ "$file" =~ (plugins|features)/com\.espressif\. ]]; then
+ echo "Signing IDF plugin/feature JAR: $file"
+
+ jarsigner \
+ -J-cp -Jjsign-7.4.jar \
+ -J--add-modules -Jjava.sql \
+ -providerClass net.jsign.jca.JsignJcaProvider \
+ -providerArg "${{ secrets.AZURE_KEYVAULT_URI }}" \
+ -keystore NONE \
+ -storetype AZUREKEYVAULT \
+ -storepass "$AZURE_TOKEN" \
+ -sigfile "$SIGFILE" \
+ -digestalg SHA-256 \
+ -tsa http://timestamp.digicert.com \
+ $CERTCHAIN_ARG \
+ -certs \
+ -verbose \
+ "$file" \
+ "${{ secrets.AZURE_KEYVAULT_CERT_NAME }}" || { echo "Failed to sign: $file"; exit 1; }
+
+ echo "Successfully signed: $file"
+ else
+ echo "Skipping non-IDF JAR: $file"
+ fi
+ done < <(find "$REPO_DIR" -type f -name "*.jar")This uses process substitution < <(...) to avoid subshell context and set -e with inline error handling to ensure failures propagate.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| find "$REPO_DIR" -type f -name "*.jar" | while read -r file; do | |
| if [[ "$file" =~ (plugins|features)/com\.espressif\. ]]; then | |
| echo "Signing IDF plugin/feature JAR: $file" | |
| jarsigner \ | |
| -J-cp -Jjsign-7.4.jar \ | |
| -J--add-modules -Jjava.sql \ | |
| -providerClass net.jsign.jca.JsignJcaProvider \ | |
| -providerArg "${{ secrets.AZURE_KEYVAULT_URI }}" \ | |
| -keystore NONE \ | |
| -storetype AZUREKEYVAULT \ | |
| -storepass "$AZURE_TOKEN" \ | |
| -sigfile "$SIGFILE" \ | |
| -digestalg SHA-256 \ | |
| -tsa http://timestamp.digicert.com \ | |
| $CERTCHAIN_ARG \ | |
| -certs \ | |
| -verbose \ | |
| "$file" \ | |
| "${{ secrets.AZURE_KEYVAULT_CERT_NAME }}" | |
| if [ $? -eq 0 ]; then | |
| echo "Successfully signed: $file" | |
| else | |
| echo "Failed to sign: $file" | |
| exit 1 | |
| fi | |
| else | |
| echo "Failed to sign: $file" | |
| exit 1 | |
| echo "Skipping non-IDF JAR: $file" | |
| fi | |
| done | |
| set -e | |
| while IFS= read -r file; do | |
| if [[ "$file" =~ (plugins|features)/com\.espressif\. ]]; then | |
| echo "Signing IDF plugin/feature JAR: $file" | |
| jarsigner \ | |
| -J-cp -Jjsign-7.4.jar \ | |
| -J--add-modules -Jjava.sql \ | |
| -providerClass net.jsign.jca.JsignJcaProvider \ | |
| -providerArg "${{ secrets.AZURE_KEYVAULT_URI }}" \ | |
| -keystore NONE \ | |
| -storetype AZUREKEYVAULT \ | |
| -storepass "$AZURE_TOKEN" \ | |
| -sigfile "$SIGFILE" \ | |
| -digestalg SHA-256 \ | |
| -tsa http://timestamp.digicert.com \ | |
| $CERTCHAIN_ARG \ | |
| -certs \ | |
| -verbose \ | |
| "$file" \ | |
| "${{ secrets.AZURE_KEYVAULT_CERT_NAME }}" || { echo "Failed to sign: $file"; exit 1; } | |
| echo "Successfully signed: $file" | |
| else | |
| echo "Skipping non-IDF JAR: $file" | |
| fi | |
| done < <(find "$REPO_DIR" -type f -name "*.jar") |
🤖 Prompt for AI Agents
.github/workflows/ci_release.yml around lines 81 to 111: the current "find ... |
while read" pipeline runs the loop in a subshell so any "exit 1" or failures
inside won't stop the parent job, and the "$? -eq 0" check after the echo
inspects the echo's exit status instead of jarsigner's. Replace the piped loop
with a loop that reads from process substitution (or otherwise avoids a
subshell) so exits propagate to the parent, and check jarsigner's exit
immediately (or use "set -e" plus "|| exit 1") right after the jarsigner
invocation instead of using "$?" after an intervening command; ensure signing
failures call "exit 1" in the same shell and that logs reflect jarsigner's
actual exit code.
Description
Please include a summary of the change and which issue is fixed.
Fixes # (IEP-XXX)
Type of change
Please delete options that are not relevant.
How has this been tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Test Configuration:
Dependent components impacted by this PR:
Checklist
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.