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

fix incomplete commit for pdns_control (re)notify #4457

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@klaus3000

klaus3000 commented Sep 11, 2016

In #3889 it was revealed that #3663 was incomplete. This pull request handles 'notify *' correctly and verifies that 'slave' is enabled.

@pieterlexis

This comment has been minimized.

Show comment
Hide comment
@pieterlexis

pieterlexis Jun 2, 2017

Member

looks good to me. @mind04 ?

Member

pieterlexis commented Jun 2, 2017

looks good to me. @mind04 ?

@mind04

This comment has been minimized.

Show comment
Hide comment
@mind04

mind04 Jun 3, 2017

Contributor

The logic is fine.
But I'm no fan of the insane verbose response, and the truckload of extra code to generate it.

10 out of 11 MASTER zones added to queue - see log. 1 out of 2 SLAVE zones added to queue - see log.

Seems nice, but only tells you "11 out of 13 domains notified, check your log for details".

Bottom line is, if you care you have to read the logs.

Contributor

mind04 commented Jun 3, 2017

The logic is fine.
But I'm no fan of the insane verbose response, and the truckload of extra code to generate it.

10 out of 11 MASTER zones added to queue - see log. 1 out of 2 SLAVE zones added to queue - see log.

Seems nice, but only tells you "11 out of 13 domains notified, check your log for details".

Bottom line is, if you care you have to read the logs.

@klaus3000

This comment has been minimized.

Show comment
Hide comment
@klaus3000

klaus3000 Jun 3, 2017

Indeed, there is some code to generate that kind of log. But that kind of log was there before - I only extended it to be "backwards compatible". I did not wanted to change anything, but fix it.

klaus3000 commented Jun 3, 2017

Indeed, there is some code to generate that kind of log. But that kind of log was there before - I only extended it to be "backwards compatible". I did not wanted to change anything, but fix it.

@klaus3000

This comment has been minimized.

Show comment
Hide comment
@klaus3000

klaus3000 Aug 8, 2017

Ping - almost one year for a simple fix. as mentioned. The unesthetic log message was there before.

klaus3000 commented Aug 8, 2017

Ping - almost one year for a simple fix. as mentioned. The unesthetic log message was there before.

@zeha zeha added this to the auth-4.2.0 milestone Jan 11, 2018

@klaus3000

This comment has been minimized.

Show comment
Hide comment
@klaus3000

klaus3000 May 4, 2018

Any plans to commit this? I am cleaning up my pull requests. So please commit or close forever.

klaus3000 commented May 4, 2018

Any plans to commit this? I am cleaning up my pull requests. So please commit or close forever.

@Habbie

This comment has been minimized.

Show comment
Hide comment
@Habbie

Habbie May 7, 2018

Member

The first commit looks good; does the second commit fix anything or does it just add logging?

Member

Habbie commented May 7, 2018

The first commit looks good; does the second commit fix anything or does it just add logging?

@klaus3000

This comment has been minimized.

Show comment
Hide comment
@klaus3000

klaus3000 May 8, 2018

The second commit is somehow "academic". When "notify *" is used, it checks if PowerDNS is configured as master or slave (or both) and then notifys only the master, slave or both domains. It also gives log output of how many master and how many slave domains were queued for notifications.

The second commit was done due to comment #3889 (comment)

klaus3000 commented May 8, 2018

The second commit is somehow "academic". When "notify *" is used, it checks if PowerDNS is configured as master or slave (or both) and then notifys only the master, slave or both domains. It also gives log output of how many master and how many slave domains were queued for notifications.

The second commit was done due to comment #3889 (comment)

@zeha zeha referenced this pull request May 29, 2018

Merged

pdns_control notify: handle slave w/o renotify properly #6691

1 of 7 tasks complete
@Habbie

This comment has been minimized.

Show comment
Hide comment
@Habbie

Habbie May 31, 2018

Member

@klaus3000 thank you for this. I'm closing this PR in favour of #6691 which contains one of the commits.

Member

Habbie commented May 31, 2018

@klaus3000 thank you for this. I'm closing this PR in favour of #6691 which contains one of the commits.

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