Add optional 'EmailFrom' configuration to NGAS Utils#107
Open
szampier wants to merge 1 commit intoICRAR:masterfrom
Open
Add optional 'EmailFrom' configuration to NGAS Utils#107szampier wants to merge 1 commit intoICRAR:masterfrom
szampier wants to merge 1 commit intoICRAR:masterfrom
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds an optional EmailFrom configuration parameter for NGAS utilities and updates xsync email sending logic to use it while preserving the existing default behavior. Sequence diagram for updated send_email EmailFrom resolutionsequenceDiagram
actor User
participant Xsync
participant NgasUtilsLib
participant NgasResourceFile
participant SmtpServer
User->>Xsync: Trigger xsync operation
Xsync->>NgasUtilsLib: send_email(subject, to, message)
NgasUtilsLib->>NgasResourceFile: get_parameter_ngas_resource_file(SmtpHost)
NgasResourceFile-->>NgasUtilsLib: smtp_host or empty
alt smtp_host is empty
NgasUtilsLib-->>Xsync: Print error No SMTP host defined in RC file
else smtp_host is defined
NgasUtilsLib->>NgasResourceFile: get_parameter_ngas_resource_file(EmailFrom)
NgasResourceFile-->>NgasUtilsLib: email_from or empty
alt EmailFrom is defined
NgasUtilsLib->>NgasUtilsLib: from_field = email_from
else EmailFrom is not defined
NgasUtilsLib->>NgasUtilsLib: from_field = getpass_user@host
end
NgasUtilsLib->>SmtpServer: Connect and send email with from_field
SmtpServer-->>User: Deliver notification email
end
Flow diagram for send_email with optional EmailFrom overrideflowchart TD
A[send_email called] --> B[Read SmtpHost from NGAS RC file]
B --> C{SmtpHost defined?}
C -- No --> D[Print error No SMTP host defined in RC file]
D --> E[Return]
C -- Yes --> F[Read EmailFrom from NGAS RC file]
F --> G{EmailFrom defined?}
G -- Yes --> H[Set from_field = EmailFrom]
G -- No --> I[Set from_field = getpass_user@host]
H --> J[Split recipient list and send via SMTP]
I --> J[Split recipient list and send via SMTP]
J --> K[End]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- When reading
NGAS_RC_PAR_EMAIL_FROM, consider normalizing the value (e.g.,strip()and verifying it is non-empty and looks like an email address) before using it, so that misconfigured or whitespace-only values don’t silently fall back or produce an invalidFromheader.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- When reading `NGAS_RC_PAR_EMAIL_FROM`, consider normalizing the value (e.g., `strip()` and verifying it is non-empty and looks like an email address) before using it, so that misconfigured or whitespace-only values don’t silently fall back or produce an invalid `From` header.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This backward-compatible change affects only the NGAS utilities (likely used only at ESO). It introduces a new optional parameter in the
~/.ngasconfiguration file to address an issue with email notifications sent by the xsync tool.Summary by Sourcery
Enhancements: