fix(ci): make weekly image-scan email step configurable and non-blocking - #1054
Conversation
There was a problem hiding this comment.
I have already done a preliminary review for you, and I hope to help you do a better job.
Summary
This PR addresses the failing "Send scan report via email" step in the weekly image scan GitHub Actions workflow by making the SMTP email step conditional on configured secrets. It reads SMTP configuration from repo secrets, skips the email step if SMTP_SERVER is missing, and adds a warning message to make the skip visible. The approach is clear and low-risk, improving the workflow's robustness without breaking existing functionality. Overall, the changes are well-scoped and improve failure handling.
Critical Issues
- Use of
secrets.SMTP_PORTwith default fallback is incorrect syntax- File:
.github/workflows/weekly-release.yaml(line ~147) - Issue: GitHub Actions expressions do not support the
||operator for default values. Using${{ secrets.SMTP_PORT || 465 }}will not work as intended and may cause an error or empty value fallback. - Suggestion: Use the
githubexpression withcontainsor theenvfallback pattern, e.g.:Or defineserver_port: ${{ secrets.SMTP_PORT != '' && secrets.SMTP_PORT || '465' }}
SMTP_PORTvia environment variable with a default in an earlier step.
- File:
Code Improvements
-
Set
SMTP_PORTin environment variables with default before the email step- File:
.github/workflows/weekly-release.yaml(lines ~104-108) - Issue: Currently SMTP_PORT is accessed directly with fallback in the action input, which is unsupported. Defining SMTP_PORT as an environment variable with a default would be clearer and reusable.
- Suggestion: Add in
env:section:Then update the email step to useSMTP_PORT: ${{ secrets.SMTP_PORT != '' && secrets.SMTP_PORT || '465' }}
server_port: ${{ env.SMTP_PORT }}.
- File:
-
Use consistent conditional syntax
- File:
.github/workflows/weekly-release.yaml(line ~139 and 157) - Issue: The condition
if: always() && env.SMTP_SERVER != ''is fine, but explicitly checking for empty strings can be fragile if the secret is unset or whitespace. - Suggestion: Consider a safer check using
env.SMTP_SERVER && always(), e.g.:This treats unset and empty as false, simplifying the condition.if: always() && env.SMTP_SERVER
- File:
-
Add a brief comment explaining the email skip logic
- File:
.github/workflows/weekly-release.yaml(near the email step) - Suggestion: Add a short comment before the email step explaining the gating on SMTP_SERVER and the fallback warning step, improving maintainability.
- File:
Best Practices
-
Add documentation for required secrets in the workflow file
- File:
.github/workflows/weekly-release.yaml(top or nearenv:) - Issue: There is no mention of the required SMTP-related secrets (
SMTP_SERVER,SMTP_USERNAME,SMTP_PASSWORD, optionalSMTP_PORT) in the workflow YAML or repo documentation. - Suggestion: Add a comment block listing required secrets and their purpose, e.g.:
# Required repo secrets for email notifications: # SMTP_SERVER - SMTP server address (e.g., smtp.example.com) # SMTP_USERNAME - SMTP username # SMTP_PASSWORD - SMTP password # SMTP_PORT - Optional SMTP port (defaults to 465)
- File:
-
Add tests or manual verification instructions for the email fallback
- File: PR description / repo documentation
- Issue: The PR notes that full email delivery verification is pending. To ensure the fallback works correctly, consider adding a manual test step or instructions for testing the workflow with and without SMTP secrets.
- Suggestion: Document in PR or README how to test this conditional email behavior or add a checklist item to verify once credentials are available.
-
Use consistent quoting style and indentation
- File:
.github/workflows/weekly-release.yaml(lines withrun:andecho "::warning::...") - Issue: The warning message uses double quotes inside a double-quoted string, which is fine but could be made clearer by using single quotes or escaping.
- Suggestion: Use single quotes for the outer string or escape quotes for clarity, e.g.:
run: echo '::warning::SMTP_SERVER secret is not configured; scan report email skipped. Set SMTP_SERVER / SMTP_USERNAME / SMTP_PASSWORD repo secrets to enable email delivery.'
- File:
Overall, this PR meaningfully improves the workflow robustness by making the email step optional and non-blocking. Addressing the critical syntax issue with the SMTP port fallback and adding some documentation will strengthen the change.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: wuhuizuo The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Summary
Fixes the failing
image-scanjob in the Weekly GitHub Release workflow (run 30724933098). The job failed at the "Send scan report via email" step withServer address must be specifiedbecause thedawidd6/action-send-mailstep never received SMTP configuration.Changes
SMTP_SERVER,SMTP_USERNAME,SMTP_PASSWORD, optionalSMTP_PORT)env.SMTP_SERVER != '': when SMTP is not configured the email step is skipped instead of failing the whole weekly runTesting
Risk
Low. When SMTP secrets are configured, behavior matches the original intent (email report); otherwise the step is skipped with a warning instead of failing the job.