-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Integrate email template in cli #11
Conversation
Signed-off-by: Rupal Mahajan <maharup@amazon.com>
Signed-off-by: Rupal Mahajan <maharup@amazon.com>
Signed-off-by: Rupal Mahajan <maharup@amazon.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some questions
src/arguments.js
Outdated
.default(DEFAULT_EMAIL_SUBJECT) | ||
.env(ENV_VAR.EMAIL_SUBJECT)) | ||
.addOption(new Option('--note <subject>', 'email note') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does email note
mean email body?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would users know what it means? i've not heard email note before
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does email body or message makes more sense? any other suggestion?
src/constants.js
Outdated
@@ -11,6 +11,7 @@ export const DEFAULT_WIDTH = '1680'; | |||
export const DEFAULT_MIN_HEIGHT = '600'; | |||
export const DEFAULT_FILENAME = 'opensearch-report'; | |||
export const DEFAULT_EMAIL_SUBJECT = 'This is an email containing your opensearch dashboard report'; | |||
export const DEFAULT_EMAIL_NOTE = 'Hi,<br>Here is the latest report!'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will users know to use <br>
instead of new line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure. should we add template is html somewhere in the docs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what happens if new line is provided? if doesn't work as expected maybe programmatically convert text to html?
just a thought but does this need to support reading from files for easier multi-line editing? e.g. --email-body-file ./email-body.txt
. or users are likely to use one-line notes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added logic to convert text to html and note will accept string or path to a text file which can contain verbose message.
Screen.Recording.2023-01-24.at.10.51.34.AM.mov |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left minor comments
@@ -22,11 +22,12 @@ try { | |||
// Do not set AWS_SDK_LOAD_CONFIG if aws config file is missing. | |||
} | |||
|
|||
export async function sendEmail(filename, format, sender, recipient, transport, smtphost, smtpport, smtpsecure, smtpusername, smtppassword, subject) { | |||
export async function sendEmail(filename, url, sender, recipient, transport, smtphost, smtpport, smtpsecure, smtpusername, smtppassword, subject, note) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: I feel the param list is a little bit long, we may consider to refactor it a little bit later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
created issue #12 to track this. will address this in separate PR.
Signed-off-by: Rupal Mahajan <maharup@amazon.com>
Signed-off-by: Rupal Mahajan <maharup@amazon.com>
Signed-off-by: Rupal Mahajan <maharup@amazon.com>
Signed-off-by: Rupal Mahajan <maharup@amazon.com>
.replace(/>/g, ">") | ||
.replace(/\t/g, " ") | ||
.replace(/ /g, "​ ​") | ||
.replace(/\r\n|\r|\n/g, "<br />"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this need "
and '
? ("
and '
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Description
Issues Resolved
List any issues this PR will resolve, e.g. Closes [...].
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.