You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@libcloud.apache.org by GitBox <gi...@apache.org> on 2020/12/29 23:12:16 UTC

[GitHub] [libcloud] macfreek opened a new pull request #1537: Deprecate Record.ttl in favour of Record.extra['ttl']

macfreek opened a new pull request #1537:
URL: https://github.com/apache/libcloud/pull/1537


   ## Deprecate `Record.ttl` in favour of `Record.extra['ttl']`
   
   Please read #1536 carefully. The current implementation stores a TTL for a DNS Record at two places, depending where you look in the code. This pull requests picks one of them.
   
   I added @property methods to maintain backwards compatibility, so the API remains consistent. However, it DOES change the internal data structure of a `libcloud.dns.base.Record`.
   
   ### Description
   
   ### Status
   
   Replace this: describe the PR status. Examples:
   
   - needs careful review
   
   ### Checklist (tick everything that applies)
   
   - [ ] [Code linting](http://libcloud.readthedocs.org/en/latest/development.html#code-style-guide) (required, can be done after the PR checks)
   - [X] Documentation (fixed that :) )
   - [X] [Tests](http://libcloud.readthedocs.org/en/latest/testing.html).
   - [ ] [ICLA](http://libcloud.readthedocs.org/en/latest/development.html#contributing-bigger-changes) (required for bigger changes)
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [libcloud] Kami closed pull request #1537: Deprecate Record.ttl in favour of Record.extra['ttl']

Posted by GitBox <gi...@apache.org>.
Kami closed pull request #1537:
URL: https://github.com/apache/libcloud/pull/1537


   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@libcloud.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [libcloud] macfreek edited a comment on pull request #1537: Deprecate Record.ttl in favour of Record.extra['ttl']

Posted by GitBox <gi...@apache.org>.
macfreek edited a comment on pull request #1537:
URL: https://github.com/apache/libcloud/pull/1537#issuecomment-752286927


   This seems to open a little can of worms in the unit tests.
   
   There were 3 failing unit tests. One was fixed by making a slight alteration in the code (DurableDNS). Two unit tests seemed erratic to me. I've changed the LuaDNS unit test to make more sense. Finally, the DNSPod behaviour does not make sense to me at all, and is still failing. I've created a separate issue (#1538) to deal with that. I'll add a [WIP] (work in progress) tag there until I've find a good solution there.
   
   ## DurableDNS
   
   DurableDNSTests.test_list_records() sets both the `ttl` and `extra['ttl']` parameter when creating a new record, but with different values:
   
   Previous behaviour:
   ```
   record.ttl == '3600'
   record.extra['ttl'] == 3600
   ```
   
   Since this PR only stores the TTL value once, not twice, one of these values has to take precedence.
   
   Behaviour in original PR:
   ```
   record.ttl == '3600'
   record.extra['ttl'] == '3600'
   ```
   
   However, after some considerations, I decided that the value of `record.extra['ttl']` should take precedence over `record.ttl` if both are set.
   
   Behaviour in final PR:
   ```
   record.ttl == 3600
   record.extra['ttl'] == 3600
   ```
   
   Note that this PR still only changes how the TTL is stored. The DurableDNSDriver still sets both the `ttl` and `extra['ttl']` parameter when creating a new record, but only casts the later to an `int`, while keeping the former a `str`. That might be a minor bug in the DurableDNSDriver. I have not (yet) fixed that.
   
   ## LuaDNS
   
   TTL is set to 13, but is [expected to be None when read](https://github.com/apache/libcloud/blob/6b588346083a510396c63b5d18db3012aa083071/libcloud/test/dns/test_luadns.py#L202):
   
       def test_create_record_success(self):
           LuadnsMockHttp.type = 'CREATE_RECORD_SUCCESS'
           record = self.driver.create_record(name='test.com.',
                                              zone=self.test_zone,
                                              type='A',
                                              data='127.0.0.1',
                                              extra={'ttl': 13})
           # ...
           self.assertIsNone(record.ttl)
   
   To me, this is not very logical. As a user of the library I would expect that the ttl would be set to 13, not be None. So I've updated the unit test with the new behaviour.
   
           self.assertEqual(record.ttl, 13)
   
   Old behaviour:
   ```
   record.ttl == None
   record.extra['ttl'] == 13
   ```
   
   New behaviour:
   ```
   record.ttl == 13
   record.extra['ttl'] == 13
   ```
   
   ## DNSPod
   
   The behaviour of DNSPod seems a bit erratic to me. See #1538 for details.
   
   The following unit test now fails:
   
       def test_create_record_success(self):
           DNSPodMockHttp.type = 'CREATE_RECORD_SUCCESS'
           record = self.driver.create_record(name='@', zone=self.test_zone,
                                              type='A', data='96.126.115.73',
                                              extra={'ttl': 13,
                                                     'record_line': 'default'})
   
   Old behaviour
   ```
   record.ttl == None
   record.extra['ttl'] == '600'
   ```
   
   New behaviour
   ```
   record.ttl == '600'
   record.extra['ttl'] == '600'
   ```
   
   Where `'600'` is the default TTL for the zone. The extra parameter (with `ttl: 13`) seems to be completely ignored. This PR exposes this erratic behaviour. Hence, the failed unit test. I'm hesitant to fix this particular bug in this PR (since I'm not familiar how the DNSPod expects the TTL), but I don't want to remove the unit test either.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [libcloud] macfreek edited a comment on pull request #1537: Deprecate Record.ttl in favour of Record.extra['ttl']

Posted by GitBox <gi...@apache.org>.
macfreek edited a comment on pull request #1537:
URL: https://github.com/apache/libcloud/pull/1537#issuecomment-752286927


   This seems to open a little can of worms in the unit tests.
   
   There are now 3 failing unit tests, most of which are caused by 
   The following excerpts are from the current unit tests. Please carefully check if this is really what is intended:
   
   ## LuaDNS
   
   TTL is set to 13, but is [expected to be None when read](https://github.com/apache/libcloud/blob/6b588346083a510396c63b5d18db3012aa083071/libcloud/test/dns/test_luadns.py#L202):
   
       def test_create_record_success(self):
           LuadnsMockHttp.type = 'CREATE_RECORD_SUCCESS'
           record = self.driver.create_record(name='test.com.',
                                              zone=self.test_zone,
                                              type='A',
                                              data='127.0.0.1',
                                              extra={'ttl': 13})
           # ...
           self.assertIsNone(record.ttl)
   
   ## DNSPod
   
   The behaviour of DNSPod seems a bit erratic to me. See #1538 for details.
   
   The following unit test now fails:
   
       def test_create_record_success(self):
           DNSPodMockHttp.type = 'CREATE_RECORD_SUCCESS'
           record = self.driver.create_record(name='@', zone=self.test_zone,
                                              type='A', data='96.126.115.73',
                                              extra={'ttl': 13,
                                                     'record_line': 'default'})
   
   Old behaviour
   ```
   record.ttl == None
   record.extra['ttl'] == '600'
   ```
   
   New behaviour
   ```
   record.ttl == '600'
   record.extra['ttl'] == '600'
   ```
   
   Where '600' is the default TTL for the zone. The extra parameter is completely ignored. This PR exposes this erratic behaviour. Hence, the failed unit test. I'm hesitant to fix this particular bug in this PR (since I'm not familiar how the DNSPod expects the TTL), but I don't want to remove the unit test either.
   
   ## DurableDNS (now fixed)
   
   DurableDNSTests.test_list_records() fails, which exposes a minor bug in this PR (which is subsequently fixed).
   
   DurableDNSTest set TTL = 3600, then reads the record, which returns '3600'.
   The DurableDNSDriver casts this to int for record.extra['ttl'], but not for record.ttl.
   
   Previous behaviour:
   ```
   record.ttl == '3600'
   record.extra['ttl'] == 3600
   ```
   
   Original PR:
   ```
   record.ttl == '3600'
   record.extra['ttl'] == '3600'
   ```
   
   Final PR:
   ```
   record.ttl == 3600
   record.extra['ttl'] == 3600
   ```


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [libcloud] macfreek edited a comment on pull request #1537: Deprecate Record.ttl in favour of Record.extra['ttl']

Posted by GitBox <gi...@apache.org>.
macfreek edited a comment on pull request #1537:
URL: https://github.com/apache/libcloud/pull/1537#issuecomment-752647557


   Removed comment -- I intended to reply in #1536.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [libcloud] macfreek commented on pull request #1537: Deprecate Record.ttl in favour of Record.extra['ttl']

Posted by GitBox <gi...@apache.org>.
macfreek commented on pull request #1537:
URL: https://github.com/apache/libcloud/pull/1537#issuecomment-752286927


   This seems to open a little can of worms in the unit tests.
   
   There are now 3 failing unit tests, most of which are caused by 
   The following excerpts are from the current unit tests. Please carefully check if this is really what is intended:
   
   ## LuaDNS
   
   TTL is set to 13, but is [expected to be None when read](https://github.com/apache/libcloud/blob/6b588346083a510396c63b5d18db3012aa083071/libcloud/test/dns/test_luadns.py#L202):
   
       def test_create_record_success(self):
           LuadnsMockHttp.type = 'CREATE_RECORD_SUCCESS'
           record = self.driver.create_record(name='test.com.',
                                              zone=self.test_zone,
                                              type='A',
                                              data='127.0.0.1',
                                              extra={'ttl': 13})
           # ...
           self.assertIsNone(record.ttl)
   
   ## DNSPod
   
   The behaviour of DNSPod seems a bit erratic to me. See #1538 for details.
   
   The following unit test now fails:
   
       def test_create_record_success(self):
           DNSPodMockHttp.type = 'CREATE_RECORD_SUCCESS'
           record = self.driver.create_record(name='@', zone=self.test_zone,
                                              type='A', data='96.126.115.73',
                                              extra={'ttl': 13,
                                                     'record_line': 'default'})
   
   Old behaviour
   record.ttl == None
   record.extra['ttl'] == '600'
   
   New behaviour
   record.ttl == '600'
   record.extra['ttl'] == '600'
   
   Where '600' is the default TTL for the zone. The extra parameter is completely ignored. This PR exposes this erratic behaviour. Hence, the failed unit test. I'm hesitant to fix this particular bug in this PR (since I'm not familiar how the DNSPod expects the TTL), but I don't want to remove the unit test either.
   
   ## DurableDNS (now fixed)
   
   DurableDNSTests.test_list_records() fails, which exposes a minor bug in this PR (which is subsequently fixed).
   
   DurableDNSTest set TTL = 3600, then reads the record, which returns '3600'.
   The DurableDNSDriver casts this to int for record.extra['ttl'], but not for record.ttl.
   
   Previous behaviour:
   ```
   record.ttl == '3600'
   record.extra['ttl'] == 3600
   ```
   
   Original PR:
   ```
   record.ttl == '3600'
   record.extra['ttl'] == '3600'
   ```
   
   Final PR:
   ```
   record.ttl == 3600
   record.extra['ttl'] == 3600
   ```


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [libcloud] stale[bot] commented on pull request #1537: Deprecate Record.ttl in favour of Record.extra['ttl']

Posted by GitBox <gi...@apache.org>.
stale[bot] commented on pull request #1537:
URL: https://github.com/apache/libcloud/pull/1537#issuecomment-868936409


   Thanks for contributing to this issue. As it has been 90 days since the last activity, we are automatically marking is as stale. If this issue is not relevant or applicable anymore (problem has been fixed in a new version or similar), please close the issue or let us know so we can close it. On the contrary, if the issue is still relevant, there is nothing you need to do, but if you have any additional details or context which would help us when working on this issue, please include it as a comment to this issue.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@libcloud.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [libcloud] Kami commented on pull request #1537: Deprecate Record.ttl in favour of Record.extra['ttl']

Posted by GitBox <gi...@apache.org>.
Kami commented on pull request #1537:
URL: https://github.com/apache/libcloud/pull/1537#issuecomment-1024955662


   Thanks for your contribution.
   
   Since this PR hasn't been updated for a long time, I will close it.
   
   If the change still relevant please feel free to re-open this PR (and in that case, please also sync it with the latest trunk).


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@libcloud.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [libcloud] Kami commented on pull request #1537: Deprecate Record.ttl in favour of Record.extra['ttl']

Posted by GitBox <gi...@apache.org>.
Kami commented on pull request #1537:
URL: https://github.com/apache/libcloud/pull/1537#issuecomment-752413458


   Thanks for opening this PR. I added my main comment here - https://github.com/apache/libcloud/issues/1536#issuecomment-752412165.
   
   Overall, I agree with you - there should only be one way of specifying it, but I think that should be ``record.ttl`` (as pointed out in that comment, that was actually a plan a while back, but we never realized it).
   
   If you agree about ``record.ttl`` being a better API, are you willing to make changes describes here https://github.com/apache/libcloud/issues/1536#issuecomment-752412165?
   
   As describes there, if we want to give our users enough heads up before a breaking change, first release will just need to include a change where all the drivers + code correctly utilize ``record.ttl``, ``record["extra"]`` still works, but it emits a deprecation warning and then in the next release fully removing it.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [libcloud] codecov-io commented on pull request #1537: Deprecate Record.ttl in favour of Record.extra['ttl']

Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #1537:
URL: https://github.com/apache/libcloud/pull/1537#issuecomment-752637206


   # [Codecov](https://codecov.io/gh/apache/libcloud/pull/1537?src=pr&el=h1) Report
   > Merging [#1537](https://codecov.io/gh/apache/libcloud/pull/1537?src=pr&el=desc) (f3da367) into [trunk](https://codecov.io/gh/apache/libcloud/commit/6b588346083a510396c63b5d18db3012aa083071?el=desc) (6b58834) will **decrease** coverage by `0.00%`.
   > The diff coverage is `88.88%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/libcloud/pull/1537/graphs/tree.svg?width=650&height=150&src=pr&token=PYoduksh69)](https://codecov.io/gh/apache/libcloud/pull/1537?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##            trunk    #1537      +/-   ##
   ==========================================
   - Coverage   83.07%   83.07%   -0.01%     
   ==========================================
     Files         394      394              
     Lines       84738    84743       +5     
     Branches     9002     9003       +1     
   ==========================================
   + Hits        70399    70401       +2     
   - Misses      11274    11276       +2     
   - Partials     3065     3066       +1     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/libcloud/pull/1537?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [libcloud/test/dns/test\_dnspod.py](https://codecov.io/gh/apache/libcloud/pull/1537/diff?src=pr&el=tree#diff-bGliY2xvdWQvdGVzdC9kbnMvdGVzdF9kbnNwb2QucHk=) | `96.91% <ø> (-0.02%)` | :arrow_down: |
   | [libcloud/dns/base.py](https://codecov.io/gh/apache/libcloud/pull/1537/diff?src=pr&el=tree#diff-bGliY2xvdWQvZG5zL2Jhc2UucHk=) | `96.52% <83.33%> (-0.76%)` | :arrow_down: |
   | [libcloud/test/dns/test\_base.py](https://codecov.io/gh/apache/libcloud/pull/1537/diff?src=pr&el=tree#diff-bGliY2xvdWQvdGVzdC9kbnMvdGVzdF9iYXNlLnB5) | `97.05% <100.00%> (-2.95%)` | :arrow_down: |
   | [libcloud/test/dns/test\_luadns.py](https://codecov.io/gh/apache/libcloud/pull/1537/diff?src=pr&el=tree#diff-bGliY2xvdWQvdGVzdC9kbnMvdGVzdF9sdWFkbnMucHk=) | `96.19% <100.00%> (ø)` | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/libcloud/pull/1537?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/libcloud/pull/1537?src=pr&el=footer). Last update [6b58834...f3da367](https://codecov.io/gh/apache/libcloud/pull/1537?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [libcloud] macfreek edited a comment on pull request #1537: Deprecate Record.ttl in favour of Record.extra['ttl']

Posted by GitBox <gi...@apache.org>.
macfreek edited a comment on pull request #1537:
URL: https://github.com/apache/libcloud/pull/1537#issuecomment-752286927


   This seems to open a little can of worms in the unit tests.
   
   There are now 3 failing unit tests, most of which are caused by 
   The following excerpts are from the current unit tests. Please carefully check if this is really what is intended:
   
   ## LuaDNS
   
   TTL is set to 13, but is [expected to be None when read](https://github.com/apache/libcloud/blob/6b588346083a510396c63b5d18db3012aa083071/libcloud/test/dns/test_luadns.py#L202):
   
       def test_create_record_success(self):
           LuadnsMockHttp.type = 'CREATE_RECORD_SUCCESS'
           record = self.driver.create_record(name='test.com.',
                                              zone=self.test_zone,
                                              type='A',
                                              data='127.0.0.1',
                                              extra={'ttl': 13})
           # ...
           self.assertIsNone(record.ttl)
   
   ## DNSPod
   
   The behaviour of DNSPod seems a bit erratic to me. See #1538 for details.
   
   The following unit test now fails:
   
       def test_create_record_success(self):
           DNSPodMockHttp.type = 'CREATE_RECORD_SUCCESS'
           record = self.driver.create_record(name='@', zone=self.test_zone,
                                              type='A', data='96.126.115.73',
                                              extra={'ttl': 13,
                                                     'record_line': 'default'})
   
   Old behaviour
   ```
   record.ttl == None
   record.extra['ttl'] == '600'
   ```
   
   New behaviour
   ```
   record.ttl == '600'
   record.extra['ttl'] == '600'
   ```
   
   Where `'600'` is the default TTL for the zone. The extra parameter (with `ttl: 13`) is completely ignored. This PR exposes this erratic behaviour. Hence, the failed unit test. I'm hesitant to fix this particular bug in this PR (since I'm not familiar how the DNSPod expects the TTL), but I don't want to remove the unit test either.
   
   ## DurableDNS (now fixed)
   
   DurableDNSTests.test_list_records() fails, which exposes a minor bug in this PR (which is subsequently fixed).
   
   DurableDNSTest set TTL = 3600, then reads the record, which returns '3600'.
   The DurableDNSDriver casts this to int for record.extra['ttl'], but not for record.ttl.
   
   Previous behaviour:
   ```
   record.ttl == '3600'
   record.extra['ttl'] == 3600
   ```
   
   Original PR:
   ```
   record.ttl == '3600'
   record.extra['ttl'] == '3600'
   ```
   
   Final PR:
   ```
   record.ttl == 3600
   record.extra['ttl'] == 3600
   ```


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [libcloud] macfreek commented on pull request #1537: Deprecate Record.ttl in favour of Record.extra['ttl']

Posted by GitBox <gi...@apache.org>.
macfreek commented on pull request #1537:
URL: https://github.com/apache/libcloud/pull/1537#issuecomment-752647557


   Hi @Kami,
   
   Thanks for your feedback, and patience -- I'm aware that opening both an issue and a PR is a bit more work for you, and you probably have noted that I have a habit to changing the issue or PR while I dive deeper into the issue. I'll try to add a "WIP" (work in progress) tag if I'm not yet happy with the report or PR. You may want to delay your comments till I remove it, to save yourself some time. (And yes, I may sometimes change it even without that tag, once I discover more quirks.)
   
   I'll have a look if I can make a PR with the suggested change. It might require a significant number of changes in the drivers though.
   
   Also, I'll have to think how and when. How to ensure a (mostly) backwards compatible API, and when to make sure this ends up a minor or major release (not a bug fix release).
   
   A second question is if you would want to consider other (perhaps breaking) changes or clarifications.
   
   A few things that come to mind are:
   * Making the priority an attribute as well. (I'm in doubt. It's fairly common, in use for MX and SRV records, but next thing you know, you'll be adding port, weight, service, and protocol too).
   * Align the different `extra` field:
     * In particular zonomi adds a `prio` field, while all other drivers use `priority` as the field name. I prefer consistency here.
     * Align the use of `description` (Google), `comment` (Rackspace) and `notes` (Zerigo).
   * Clearly specify the use of trailing dots in both name and data that contain a FQDN. Right now, the name does seems to require a trailing dot (e.g. `www.example.com.`) A data field that requires a FQDN (e.g. MX, NS, PTR or CNAME) do require a trailing dot in DNS itself, but it seems that most API supported here insist that there MUST NOT be a trailing dot.
   * Be clear if/how IDN (international domain names) are supported. I kind of suspect that both name and data fields are expected to be punycode-encoded (e.g. `xn--bcher-kva.de.` instead of `bücher.de.`).


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [libcloud] macfreek edited a comment on pull request #1537: Deprecate Record.ttl in favour of Record.extra['ttl']

Posted by GitBox <gi...@apache.org>.
macfreek edited a comment on pull request #1537:
URL: https://github.com/apache/libcloud/pull/1537#issuecomment-752286927


   This seems to open a little can of worms in the unit tests.
   
   There were 3 failing unit tests. One was fixed by making a slight alteration in the code (DurableDNS). Two unit tests seemed erratic to me. I've changed the LuaDNS unit test to make more sense. Finally, the DNSPod behaviour does not make sense to me at all, and is still failing. I've created a separate issue (#1538) to deal with that. I'll add a [WIP] (work in progress) tag there until I've find a good solution there.
   
   ## DurableDNS
   
   DurableDNSTests.test_list_records() sets both the `ttl` and `extra['ttl']` parameter when creating a new record, but with different values:
   
   Previous behaviour:
   ```
   record.ttl == '3600'
   record.extra['ttl'] == 3600
   ```
   
   Since this PR only stores the TTL value once, not twice, one of these values has to take precedence.
   
   Behaviour in original PR:
   ```
   record.ttl == '3600'
   record.extra['ttl'] == '3600'
   ```
   
   However, after some considerations, I decided that the value of `record.extra['ttl']` should take precedence over `record.ttl` if both are set.
   
   Behaviour in final PR:
   ```
   record.ttl == 3600
   record.extra['ttl'] == 3600
   ```
   
   Note that this PR still only changes how the TTL is stored. The DurableDNSDriver still sets both the `ttl` and `extra['ttl']` parameter when creating a new record, but only casts the later to an `int`, while keeping the former a `str`. That might be a minor bug in the DurableDNSDriver. I have not (yet) fixed that.
   
   ## LuaDNS
   
   TTL is set to 13, but is [expected to be None when read](https://github.com/apache/libcloud/blob/6b588346083a510396c63b5d18db3012aa083071/libcloud/test/dns/test_luadns.py#L202):
   
       def test_create_record_success(self):
           LuadnsMockHttp.type = 'CREATE_RECORD_SUCCESS'
           record = self.driver.create_record(name='test.com.',
                                              zone=self.test_zone,
                                              type='A',
                                              data='127.0.0.1',
                                              extra={'ttl': 13})
           # ...
           self.assertIsNone(record.ttl)
   
   To me, this is not very logical. As a user of the library I would expect that the ttl would be set to 13, not be None. So I've updated the unit test with the new behaviour.
   
   Old behaviour:
   ```
   record.ttl == None
   record.extra['ttl'] == 13
   ```
   
   New behaviour:
   ```
   record.ttl == 13
   record.extra['ttl'] == 13
   ```
   
   ## DNSPod
   
   The behaviour of DNSPod seems a bit erratic to me. See #1538 for details.
   
   The following unit test now fails:
   
       def test_create_record_success(self):
           DNSPodMockHttp.type = 'CREATE_RECORD_SUCCESS'
           record = self.driver.create_record(name='@', zone=self.test_zone,
                                              type='A', data='96.126.115.73',
                                              extra={'ttl': 13,
                                                     'record_line': 'default'})
   
   Old behaviour
   ```
   record.ttl == None
   record.extra['ttl'] == '600'
   ```
   
   New behaviour
   ```
   record.ttl == '600'
   record.extra['ttl'] == '600'
   ```
   
   Where `'600'` is the default TTL for the zone. The extra parameter (with `ttl: 13`) seems to be completely ignored. This PR exposes this erratic behaviour. Hence, the failed unit test. I'm hesitant to fix this particular bug in this PR (since I'm not familiar how the DNSPod expects the TTL), but I don't want to remove the unit test either.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org