Skip to content
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

Notification setup #6

Merged
merged 12 commits into from
Feb 8, 2020
Merged

Notification setup #6

merged 12 commits into from
Feb 8, 2020

Conversation

Akasora39
Copy link
Collaborator

  • Add script for installing Node.js v13 and initializing notifications directory
  • Demonstrate basic functionality of nodemailer

@Akasora39 Akasora39 added the enhancement New feature or request label Feb 5, 2020
@Akasora39 Akasora39 self-assigned this Feb 5, 2020
service: 'Gmail',
auth: {
user: '[email protected]',
pass: 'Thisisapassword'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use an environment variable for password and change password

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes this is a big oversight on my part. Thanks for pointing it out.


# Install Node.js on Linux as root on RHEL, CentOS, CloudLinux, or Fedora
install_node() {
if which node > /dev/null
Copy link
Collaborator

Choose a reason for hiding this comment

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

Adopt the command -v form seen in other install scripts to check if program exists

@@ -0,0 +1,19 @@
#!/bin/bash
Copy link
Collaborator

@yliu0715 yliu0715 Feb 6, 2020

Choose a reason for hiding this comment

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

library files should not have this line as noted previously
https://google.github.io/styleguide/shell.xml?showone=File_Extensions#File_Extensions

scripts/install Show resolved Hide resolved
scripts/lib/install_node.sh Outdated Show resolved Hide resolved
command -v node >/dev/null 2>&1 || {
curl -o- https://raw.githubusercontent.com/nvm-sh/nvm/v0.35.2/install.sh | bash
source ~/.nvm.sh
nvm install node
Copy link
Collaborator

Choose a reason for hiding this comment

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

I usually install the specific version of node like nvm install 13.8 and then nvm use 13.8. It's better to install the specific version of node to remove any bugs associated with different node versions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Makes sense. I think 12.15.0 would be suitable for this as it's the latest stable version.

@Akasora39 Akasora39 requested a review from chc5 February 7, 2020 15:30
scripts/lib/install_node.sh Outdated Show resolved Hide resolved
scripts/lib/install_node.sh Outdated Show resolved Hide resolved
@Akasora39 Akasora39 requested a review from chc5 February 7, 2020 17:25
@chc5
Copy link
Collaborator

chc5 commented Feb 7, 2020

LTGM


# Install dependencies for notification folder
init_notif() {
cd ../src/notifications && npm install
Copy link
Collaborator

@melvinheks melvinheks Feb 7, 2020

Choose a reason for hiding this comment

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

Add cd - to return to previous directory. I.e. ensure that the current working directory is the same as when the install function is called.

@Akasora39 Akasora39 merged commit 248e354 into master Feb 8, 2020
@meowl-surveillance-system meowl-surveillance-system deleted the notification-setup branch March 5, 2020 02:52
@meowl-surveillance-system meowl-surveillance-system restored the notification-setup branch March 5, 2020 02:52
@meowl-surveillance-system meowl-surveillance-system deleted the notification-setup branch March 5, 2020 02:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants