Skip to content
This repository has been archived by the owner on Feb 7, 2020. It is now read-only.

Jeversmann/success mail #89

Merged
merged 2 commits into from
Jun 24, 2014
Merged

Jeversmann/success mail #89

merged 2 commits into from
Jun 24, 2014

Conversation

jeversmann
Copy link
Contributor

Sends build success emails when developer builds and pull requests pass.

mail :to => @emails,
:bcc => Settings.kochiku_notifications_email_address,
:subject => "[kochiku] #{@build.project.name} pull request approved",
:from => "#{email_user}+#{@build.project.name.parameterize}@#{email_domain}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it makes sense to just use sender_email_address as is. At minimum it should be repository name instead of project name but I think I'd rather have neither. We can change the build_break_email to match.

@robolson
Copy link
Collaborator

Missing option at the repository level to control whether build success emails get sent.


def build_success_email(build)
@build = build
@emails = GitBlame.emails_in_branch(@build)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does it make sense to send the success email to every person who has a commit on the branch?

Ideally we would notify the person who submitted the PR or the developer build but we do not actually have that information.

In lieu of that I think the best thing we can do is notify the person who has the most recent commit on the branch.

@jeversmann
Copy link
Contributor Author

I went ahead and changed all of the GitBlame methods to use committer instead of author.

expect(email.html_part.body).to include(build_part.project.name)
expect(email.text_part.body).to include(build_part.project.name)
expect(email.html_part.body).to include("http://")
expect(email.text_part.body).to include("http://")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't really think these last 4 expects provide any value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They're there to be consistent with the content of the other BuildMailer specs, and I don't think it's worth removing them from all of the descriptions.

@robolson
Copy link
Collaborator

LGTM to me. Let's clean up the commits before merging.

@jeversmann
Copy link
Contributor Author

Rebased onto master after #91 was merged.

robolson added a commit that referenced this pull request Jun 24, 2014
@robolson robolson merged commit 96aa676 into master Jun 24, 2014
@robolson robolson deleted the jeversmann/success_mail branch June 24, 2014 21:52
JackDanger pushed a commit that referenced this pull request Oct 29, 2015
…specialness to master

* commit 'd75c163e865cdfe16134d31a1df7190affd13c34':
  fix typo
  remove maven partitioner special case for the all-protos/protos git subtree
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants