Inconsistency in specifying TTL in DNS records between example and tests
- Dominant language
- Python
- Stars
- 2.1k
- Forks
- 931
- Avg merge
- 1d 2h
- Merged PRs (30d)
- 4
Description
## Summary
Right now, the TTL for individual DNS Records is stored either in the `Record.ttl` attribute, or in the `Record.extra` dict, or in both.
Some of the code stores or access the `Record.ttl` value, other code the `Record.extra['ttl']` value. I'll give two examples of either.
This inconsistency causes bugs. E.g. if you follow the documentation for [Create a record with a custom TTL](https://libcloud.readthedocs.io/en/stable/dns/examples.html#create-a-record-with-a-custom-ttl), and subsequently call `libcloud.dns.base.DNSDriver.export_zone_to_bind_format()`, it will **not** output the specified TTL, but the default TTL of the zone.
## Detailed Information
First of all, note that this only concerns a custom TTL in a `libcloud.dns.base.Record`. This is not commonly set. Usually, records used the default TTL as specified in the associated `libcloud.dns.base.Zone`.
### TTL stored in the `Record.ttl` attribute
In the documentation, at [Create a record with a custom TTL](https://libcloud.readthedocs.io/en/stable/dns/examples.html#create-a-record-with-a-custom-ttl), a custom TTL is given as parameter to Record constructor:
```
ttl = 900
record = zone.create_record(name='www', type=RecordType.A, data='127.0.0.1',
ttl=ttl)
```
In the [`Record.__init__()` method, on line 163](https://github.com/apache/libcloud/blob/6b588346083a510396c63b5d18db3012aa083071/libcloud/dns/base.py#L163) this ttl is stored in the `Record.ttl` attribute:
```
self.ttl = ttl
```
### TTL stored in `Record.extra["ttl"]`
On the other hand, in the [libcloud/test/dns/test_base.py unit test, on line 38](https://github.com/apache/libcloud/blob/6b588346083a510396c63b5d18db3012aa083071/libcloud/test/dns/test_base.py#L38), the TTL is not set using the `ttl` parameter, but using the `extra` parameter.
values = {'id': 3, 'name': 'www', 'type': RecordType.A, 'data': '127.0.0.1',
'extra': {'ttl': 123}},
# ...
record = Record(**values)
Futhermore, in the [`DNSDriver. _get_bind_record_line()` method, on line 549](https://github.com/apache/libcloud/blob/6b588346083a510396c63b5d18db3012aa083071/libcloud/dns/base.py#L549) the ttl is retrieved using the `extra` attribute, ignoring the `ttl` attribute of the `Record` (it does look at the `Zone.ttl` though). Finally, this last behaviour is checked in the unit test.
ttl = record.extra['ttl'] if 'ttl' in record.extra else record.zone.ttl
## How to proceed?
My first attempt to fix this issue was in pull request #1535. In there, I changed the `DNSDriver. _get_bind_record_line()` to use the `Record.ttl` instead of the `Record.extra["ttl"]`
I now think that fixing it the other way around it better: deprecating `Record.ttl` in favour of the `Record.extra["ttl"]`.
I have two main arguments:
1. The sample code is incorrect. It calls `zone.create_record()` with `ttl` as a parameter. But [`Zone.create_record()`](https://github.com/apache/libcloud/blob/6b588346083a510396c63b5d18db3012aa083071/libcloud/dns/base.py#L83) does not have a `ttl` parameter! Only the `Record.__init__()` method does. So this sample code will fail with a `TypeError` exception.
2. Most code seem to use the `Records.extra["ttl"]`, not the `Record.ttl`. However, I have not done an extensive check yet. (neither of the libcloud code, nor of code out in the wild that uses libcloud).
## Reaching consensus
* Do you concur that it is better to store the TTL at only one place, not two?
* Do you concur that it is best to store it in `Records.extra["ttl"]`, while creating wrapper functions around `Record.ttl `for backward compatibility?
If so, please have a look at PR #1537.
Contributor guide
Research direction
Read libcloud/dns/base.py and libcloud/test/dns/test_base.py first, then compare the DNS documentation example with the Record constructor, Zone.create_record(), and DNSDriver._get_bind_record_line(). Done means the project has an agreed single TTL representation, consistent documentation and tests, and an explicit backward-compatibility outcome.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python
- Domain
- api, networking
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 30/100