dns-update-system-records: add support for nsupdate output format #423
Conversation
ipaclient/plugins/dns.py
Outdated
| """Store data in nsupdate format in file""" | ||
| def parse_rname_rtype(record): | ||
| """Get rname and rtype from textual representation of record""" | ||
| l = record.split(' ', 5) |
There was a problem hiding this comment.
If we're limiting the amount of splits, I think 4 is the correct number here.
ipaclient/plugins/dns.py
Outdated
|
|
||
| already_removed = set() | ||
| for key in self.record_groups: | ||
| if result.get(key): |
There was a problem hiding this comment.
Is it possible to use key in result here? It would improve readability.
There was a problem hiding this comment.
not fixed, reverting. I did that on purpose to not generate empty location records
if key in result and result[key]:
can be replaced by
if result.get(key):
ipaclient/plugins/dns.py
Outdated
| rname=r_name_type[0], rtype=r_name_type[1] | ||
| )) | ||
| # add new | ||
| file.write("update add {}\n".format(val)) |
There was a problem hiding this comment.
What happens if update add ... is ran twice for the same record?
There was a problem hiding this comment.
This shouldn't happen
There was a problem hiding this comment.
So the command update delete ... will remove for example all A records for a domain and that's why you're checking that it won't run twice. However, when the records are added, they should be all different (in case of A record, different IPv4). Is that correct?
There was a problem hiding this comment.
yes IPA doesn't return duplicated records
ipaclient/plugins/dns.py
Outdated
|
|
||
| out = options.get('out') # output to file | ||
| if out: | ||
| with open(out, "w") as f: |
There was a problem hiding this comment.
I think it'd be nice if we had try-catch block here to handle potential errors with file operations.
There was a problem hiding this comment.
ok, I'll use the same code as cert.py does for certificates
|
I added some in-line comments/questions. |
ipaclient/plugins/dns.py
Outdated
| 'out?', | ||
| include='cli', | ||
| doc=_('file to store DNS records in nsupdate format') | ||
| ) |
There was a problem hiding this comment.
Not that this won't work, but you can use takes_options as usual here, MethodOverride will tak care of properly merging the remote and local options.
There was a problem hiding this comment.
I did exactly the same as was used in user.py plugin
ipaclient/plugins/dns.py
Outdated
| with open(out, "w") as f: | ||
| self._nsupdate_output_file(f, result, labels) | ||
| except (OSError, IOError) as e: | ||
| raise errors.FileError(reason=unicode(e)) |
There was a problem hiding this comment.
This block should be moved to forward(), output_for_cli() is supposed to just format the result on the CLI, not write stuff to files.
|
I've added acceptance criteria and user story to the related FreeIPA ticket. I miss a "how to use part" - a specific example. This should be in FreeIPA.org wiki, e.g. in design page (rest of the design page can be copied user story and empty), but the how to use section with both auth methods is a critical part. |
Added option --out <path> creates a file with IPA DNS data in nsupdate format. https://fedorahosted.org/freeipa/ticket/6585
Get nsupdate data from dns-update-system-records, remove system records and run nsupdate to verify that all system records were updated https://fedorahosted.org/freeipa/ticket/6585
|
@pvoborni http://www.freeipa.org/page/Howto/Updating_FreeIPA_system_DNS_records_on_a_remote_DNS_server Still WIP, but can be reviewed if format fulfill expectations. |
|
Please update the ticket in trac/JIRA to mentiond the command does not support stdout. LGTM otherwise. |
Option --out does the trick