Join GitHub today
GitHub is home to over 28 million developers working together to host and review code, manage projects, and build software together.
Sign upFixes #18549 - Don't rebuild already correct DNS entries #4297
Conversation
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
mention-bot
Feb 17, 2017
@agx, thanks for your PR! By analyzing the history of the files in this pull request, we identified @ohadlevy, @isratrade and @timogoebel to be potential reviewers.
mention-bot
commented
Feb 17, 2017
|
@agx, thanks for your PR! By analyzing the history of the files in this pull request, we identified @ohadlevy, @isratrade and @timogoebel to be potential reviewers. |
theforeman-bot
added
Needs testing
Not yet reviewed
labels
Feb 17, 2017
dLobatog
requested changes
Feb 17, 2017
@agx One minor comment inline, please check it out. Thanks for the contribution!
| @@ -42,7 +42,7 @@ def rebuild_dns | ||
| results = {} | ||
| DnsInterface::RECORD_TYPES.each do |record_type| | ||
| del_dns_record_safe(record_type) | ||
| dns_record(record_type).nil? || dns_record(record_type).valid? || del_dns_record_safe(record_type) |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
dLobatog
Feb 17, 2017
Member
The first check for .nil? is already done on del_dns_record_safe on app/model/concerns/dns_interface.rb.
Could you move the .valid? check to that method, and add a test on test/models/orchestration/dns_test.rb for rebuild_dns with an already correct DNS record?
dLobatog
Feb 17, 2017
Member
The first check for .nil? is already done on del_dns_record_safe on app/model/concerns/dns_interface.rb.
Could you move the .valid? check to that method, and add a test on test/models/orchestration/dns_test.rb for rebuild_dns with an already correct DNS record?
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
agx
Feb 22, 2017
Member
@dLobatog wouldn't that obfuscate the meaning of del_dns_record_safe? It'd rather add a del_incorrect_dns_record
agx
Feb 22, 2017
Member
@dLobatog wouldn't that obfuscate the meaning of del_dns_record_safe? It'd rather add a del_incorrect_dns_record
theforeman-bot
added
Waiting on contributor
and removed
Not yet reviewed
labels
Feb 17, 2017
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
theforeman-bot
Jun 27, 2018
Member
Thank you for your contribution, @agx! This PR has been inactive for 6 months, closing for now.
Feel free to reopen when you return to it. This is an automated process.
|
Thank you for your contribution, @agx! This PR has been inactive for 6 months, closing for now. |
agx commentedFeb 17, 2017
If we do so and try to recreate them right away ecreate_dns_record might
still see a valid entry due to zone transfer slowness or other caching
issues. In this case we'd end up without a DNS entry after all. So
intead do nothing if the entry is already good.
This is a forward port from a fix I applied to a local 1.10.x instance.
Signed-off-by: Guido Günther agx@sigxcpu.org