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

Email alerts #287

Merged
merged 11 commits into from
Mar 13, 2017
Merged

Email alerts #287

merged 11 commits into from
Mar 13, 2017

Conversation

strangeman
Copy link
Contributor

See #269

Please, look precisely to migration and to api/tasks/alert.go (especially on work with template - I think this code can be better, but my Golang-fu isn't perfect :( ).

Copy link
Contributor

@matejkramny matejkramny left a comment

Choose a reason for hiding this comment

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

Code's good, thank you.

There are some small changes. The code for templates looks ok.

}

func (t *task) sendMailAlert() {

Copy link
Contributor

Choose a reason for hiding this comment

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

this code block (not specific to Go) could be rewritten to be more readable

Example:

if true {
  do something
} else {
  do the other stuff
}

Could be rewritten to be more expressive as:

if false {
  do something
  return
}

do the other stuff

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

}
}
} else {
t.log("Alerts disabled for this project, nothing to do.")
Copy link
Contributor

Choose a reason for hiding this comment

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

if alerts are disabled, there's no need to log them :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@@ -7,8 +7,12 @@ form.form-horizontal
input.form-control(type="text" ng-model="projectName" placeholder="Project Name")

.form-group
label.control-label.col-sm-4 Allow alerts for this project
.col-sm-1: input.form-control(type="checkbox" title="Send email alerts about failed tasks" ng-model="alert")
Copy link
Contributor

Choose a reason for hiding this comment

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

See the docs for checkboxes in bootstrap: .checkbox class

Copy link
Contributor Author

Choose a reason for hiding this comment

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

made it a bit less ugly

util/config.go Outdated
@@ -182,4 +191,44 @@ func (conf *configType) Scan() {
conf.TmpPath = "/tmp/semaphore"
}
conf.TmpPath = path.Clean(conf.TmpPath)

var alertanswer string
fmt.Print(" > Do you need an email alerts (y/n, default n): ")
Copy link
Contributor

Choose a reason for hiding this comment

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

change to > Enable email alerts (y/n, default n): please

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

util/mail.go Outdated
"net/smtp"
)

func SendMail(emailHost string, mailSender string, mailRecipient string, mail bytes.Buffer) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

tip: func SendMail (emailHost, mailSender, mailRecipient string, mail bytes.Buffer) error {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@@ -0,0 +1,2 @@
ALTER TABLE user ADD alert BOOLEAN NOT NULL AFTER password;
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure if BOOLEAN type is new, check compatibility with versions of mysql/mariadb listed in wiki

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BOOLEAN supported at least since MySQL 5.5 - https://dev.mysql.com/doc/refman/5.5/en/numeric-type-overview.html

@strangeman
Copy link
Contributor Author

@matejkramny Thank you for the review! I'll update my PR soon.

@strangeman
Copy link
Contributor Author

Changes are made. One question: should I provide some documentation and where should I put it? In the Wiki?

@matejkramny
Copy link
Contributor

Hmm there's no documentation (aside from the swagger API spec).

If there are any API changes, put it in the swagger document

@strangeman
Copy link
Contributor Author

If there are any API changes, put it in the swagger document

done

@matejkramny
Copy link
Contributor

Awesome, thanks!

@matejkramny matejkramny merged commit 7b457df into semaphoreui:master Mar 13, 2017
@strangeman strangeman deleted the email-alerts branch March 13, 2017 23:58
matejkramny added a commit that referenced this pull request Apr 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants