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

Use parent instead of self.parent in MailNotifier.setServiceParent() #1525

Merged
merged 1 commit into from Feb 5, 2015

Conversation

Projects
None yet
4 participants
@ewongbb
Contributor

ewongbb commented Feb 5, 2015

MailNotifier's setServiceParent() was setting self.master_status to self.parent, which doesn't exist and
I'm guessing the intention is to set it to the parameter parent.

@tardyp

This comment has been minimized.

Show comment
Hide comment
@tardyp

tardyp Feb 5, 2015

Member

I guess this would require a little bit more attention. Looking at the code, the master_status is still not fully mq and data aware.

We have the choice to either keep the master_status api, and make it baked by the data and mq api, or completely remove the master_status, and make all status plugin directly use data and mq api.

I would be more in favour of to the second option as I fear that the old status api is too much synchronous to really be backed by data.

Member

tardyp commented Feb 5, 2015

I guess this would require a little bit more attention. Looking at the code, the master_status is still not fully mq and data aware.

We have the choice to either keep the master_status api, and make it baked by the data and mq api, or completely remove the master_status, and make all status plugin directly use data and mq api.

I would be more in favour of to the second option as I fear that the old status api is too much synchronous to really be backed by data.

@tardyp

This comment has been minimized.

Show comment
Hide comment
@tardyp
Member

tardyp commented Feb 5, 2015

@sa2ajj sa2ajj added the please review label Feb 5, 2015

@sa2ajj

This comment has been minimized.

Show comment
Hide comment
@sa2ajj

sa2ajj Feb 5, 2015

Contributor

I'd say that this particula change is about a typo/overlooked change and I'm very much inclined to merge it as is.

Contributor

sa2ajj commented Feb 5, 2015

I'd say that this particula change is about a typo/overlooked change and I'm very much inclined to merge it as is.

@tardyp

This comment has been minimized.

Show comment
Hide comment
@tardyp

tardyp Feb 5, 2015

Member

OK

Le jeu. 5 févr. 2015 13:32, Mikhail Sobolev notifications@github.com a
écrit :

I'd say that this particula change is about a typo/overlooked change and
I'm very much inclined to merge it as is.


Reply to this email directly or view it on GitHub
#1525 (comment).

Member

tardyp commented Feb 5, 2015

OK

Le jeu. 5 févr. 2015 13:32, Mikhail Sobolev notifications@github.com a
écrit :

I'd say that this particula change is about a typo/overlooked change and
I'm very much inclined to merge it as is.


Reply to this email directly or view it on GitHub
#1525 (comment).

sa2ajj added a commit that referenced this pull request Feb 5, 2015

Merge pull request #1525 from ewongbb/ticket3182
Use parent instead of self.parent in MailNotifier.setServiceParent()

@sa2ajj sa2ajj merged commit ca971a6 into buildbot:master Feb 5, 2015

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@sa2ajj

This comment has been minimized.

Show comment
Hide comment
@sa2ajj

sa2ajj Feb 5, 2015

Contributor

Thank you, @ewongbb :)

Contributor

sa2ajj commented Feb 5, 2015

Thank you, @ewongbb :)

@djmitche

This comment has been minimized.

Show comment
Hide comment
@djmitche

djmitche Feb 6, 2015

Member

Nice work -- Pierre's right, we need to fix this stuff, but this patch is good.

Member

djmitche commented Feb 6, 2015

Nice work -- Pierre's right, we need to fix this stuff, but this patch is good.

@ewongbb ewongbb deleted the ewongbb:ticket3182 branch Feb 10, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment