Skip to content

fix(ci): make weekly image-scan email step configurable and non-blocking - #1054

Merged
ti-chi-bot[bot] merged 1 commit into
mainfrom
fix/weekly-image-scan-email
Aug 10, 2026
Merged

fix(ci): make weekly image-scan email step configurable and non-blocking#1054
ti-chi-bot[bot] merged 1 commit into
mainfrom
fix/weekly-image-scan-email

Conversation

@wuhuizuo

@wuhuizuo wuhuizuo commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes the failing image-scan job in the Weekly GitHub Release workflow (run 30724933098). The job failed at the "Send scan report via email" step with Server address must be specified because the dawidd6/action-send-mail step never received SMTP configuration.

Changes

  • Read SMTP server/credentials from repo secrets (SMTP_SERVER, SMTP_USERNAME, SMTP_PASSWORD, optional SMTP_PORT)
  • Gate the email step on env.SMTP_SERVER != '': when SMTP is not configured the email step is skipped instead of failing the whole weekly run
  • Add a fallback step that emits a warning when the report email is skipped, so the condition is visible in logs

Testing

  • YAML validated with js-yaml
  • Workflow only runs on schedule/manual trigger; the email step now degrades gracefully when secrets are absent, so the next scheduled run will pass regardless of secret availability. Full email delivery verification pending SMTP credentials.

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.

@ti-chi-bot ti-chi-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_PORT with 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 github expression with contains or the env fallback pattern, e.g.:
      server_port: ${{ secrets.SMTP_PORT != '' && secrets.SMTP_PORT || '465' }}
      Or define SMTP_PORT via environment variable with a default in an earlier step.

Code Improvements

  • Set SMTP_PORT in 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:
      SMTP_PORT: ${{ secrets.SMTP_PORT != '' && secrets.SMTP_PORT || '465' }}
      Then update the email step to use server_port: ${{ env.SMTP_PORT }}.
  • 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.:
      if: always() && env.SMTP_SERVER
      This treats unset and empty as false, simplifying the condition.
  • 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.

Best Practices

  • Add documentation for required secrets in the workflow file

    • File: .github/workflows/weekly-release.yaml (top or near env:)
    • Issue: There is no mention of the required SMTP-related secrets (SMTP_SERVER, SMTP_USERNAME, SMTP_PASSWORD, optional SMTP_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)
  • 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 with run: and echo "::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.'

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.

@ti-chi-bot ti-chi-bot Bot added the size/S label Aug 9, 2026

@wuhuizuo wuhuizuo left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve

@ti-chi-bot

ti-chi-bot Bot commented Aug 10, 2026

Copy link
Copy Markdown

[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

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot Bot added the approved label Aug 10, 2026
@ti-chi-bot
ti-chi-bot Bot merged commit 5a2486a into main Aug 10, 2026
4 checks passed
@ti-chi-bot
ti-chi-bot Bot deleted the fix/weekly-image-scan-email branch August 10, 2026 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant