Skip to content
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

dns: prompt for missing record parts in CLI #34

Closed
wants to merge 3 commits into from
Closed

dns: prompt for missing record parts in CLI #34

wants to merge 3 commits into from

Conversation

HonzaCholasta
Copy link
Contributor

@HonzaCholasta HonzaCholasta commented Aug 29, 2016

Fix the code which determines if a record part is required and thus should
be prompted not to wrongfully consider all record parts to be optional.

Add a client-side fallback of the dnsrecord_split_parts command for old
servers to avoid CommandError in dnsrecord_add and dnsrecord_mod CLI
interactive mode.

https://fedorahosted.org/freeipa/ticket/6203

@MartinBasti
Copy link
Contributor

I really don't like to move definitions how to split params from classes to single function

def split_rrparam(name, value):

It doesn't look safe for me or easy to understand and maintain. When I want to add new DNS type, I have to check 3 different files, I'm sure I will overlook something.

@HonzaCholasta
Copy link
Contributor Author

I'm afraid this can't be easily fixed due to the manner in which the dns plugin is implemented.

I'm open to suggestions if you have any.

@MartinBasti
Copy link
Contributor

Can we move DNS record classes to ipalib/dnsrecords.py and use it in both client and server?

IMO for server side code it should work just fine, not sure about client.

@HonzaCholasta
Copy link
Contributor Author

I have decided to instead copy & paste the code, as it exists solely for the purpose of supporting old servers, so it should not get any additional improvements in the future.

@MartinBasti MartinBasti self-assigned this Sep 1, 2016
@MartinBasti
Copy link
Contributor

Fix works for me partially, it fixes issues reported in ticket. Do you want to open new ticket for this or should it be part of this ticket?

Expected:

[root@vm-058-080 ~]# ipa dnsrecord-add test. rec
Please choose a type of DNS resource record to be added
The most common types for this type of zone are: A, AAAA

DNS resource record type: srv
SRV Priority: 5
SRV Weight: 5
SRV Port: 5
SRV Target: host.example.com.
  Record name: rec
  SRV record: 5 5 5 host.example.com.

Got:

[root@vm-058-017 ~]# ipa dnsrecord-add test. rec
Please choose a type of DNS resource record to be added
The most common types for this type of zone are: A, AAAA

DNS resource record type: srv
ipa: ERROR: No options to add a specific record provided.
Command help may be consulted for all supported record types.

When dnsrecord_add is called without options in interactive mode, it
prompts the user to enter a record type. The record type is expected to be
upper case further in the code, which causes non-upper case values not to
work correctly.

Fix this issue by upper casing the value after it is read.

https://fedorahosted.org/freeipa/ticket/6203
@MartinBasti
Copy link
Contributor

IMO api version should be incremented, otherwise works for me

Jan Cholasta added 2 commits September 6, 2016 07:26
Fix the code which determines if a record part is required and thus should
be prompted not to wrongfully consider all record parts to be optional.

https://fedorahosted.org/freeipa/ticket/6203
Add a client-side fallback of the dnsrecord_split_parts command for old
servers to avoid CommandError in dnsrecord_add and dnsrecord_mod CLI
interactive mode.

https://fedorahosted.org/freeipa/ticket/6203
@MartinBasti MartinBasti added ack Pull Request approved, can be merged pushed Pull Request has already been pushed labels Sep 6, 2016
@MartinBasti MartinBasti closed this Sep 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ack Pull Request approved, can be merged pushed Pull Request has already been pushed
Projects
None yet
2 participants