apache / apache/libcloud

Inconsistency in specifying TTL in DNS records between example and tests

Offen
#1,536 4 Kommentare 0 Reaktionen 0 zugewiesene Personen Auf GitHub ansehen
stale
Vorherrschende Sprache
Python
Sterne
2.1k
Forks
931
Ø Merge
1 T. 2 Std.
Gemergte PRs (30 T.)
4

Beschreibung

## 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.

Beitragsleitfaden

Beitragsleitfaden öffnen

Rechercherichtung

Lies zuerst libcloud/dns/base.py und libcloud/test/dns/test_base.py und vergleiche anschließend das DNS-Dokumentationsbeispiel mit dem Record-Konstruktor, Zone.create_record() und DNSDriver._get_bind_record_line(). Als erledigt gilt, dass das Projekt eine einheitliche TTL-Darstellung vereinbart hat, Dokumentation und Tests konsistent sind und das Ergebnis der Abwärtskompatibilität explizit festgehalten ist.

Vom Indexierungsmodell aus dem Issue-Text verfasst.

Bewertung

Tech-Stack
python
Bereich
api, networking
Issue-Typ
Bug
Schwierigkeit
5/5
Geschätzter Aufwand
Über eine Woche
Aktivitätsstatus
Veraltet
Klarheit
Größtenteils klar
Anfängerfreundlichkeit
30/100

Neue Issues direkt in Ihr Postfach

Eine kurze Übersicht über anfängerfreundliche GitHub-Issues.