Open max-arnold opened 5 years ago
I'll pick this up
All issues raised fixed in #50296
TBH, this could also do with an integration test module. I've added a load of unit tests in the execution module which found all the bugs you raised (and a couple of others), mainly around kwarg naming and the extra
dictionary.
@max-arnold Thank you for reporting this.
@tonybaloney Thanks for the PR and quick turn around.
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
If this issue is closed prematurely, please leave a comment and we will gladly reopen the issue.
Still relevant
Thank you for updating this issue. It is no longer marked as stale.
Description of Issue/Question
I found so many issues while trying to use
libcloud_dns
, that it looks like it was never tested at all.profile
andprovider
is really confusing, and it looks like I'm not the only one who spent hours on this: #44390salt-cloud
docs also mentionlibcloud
and providers/profiles https://docs.saltstack.com/en/latest/topics/cloud/#configuration-inheritance, so these features are easy to misunderstand.https://docs.saltstack.com/en/latest/ref/states/all/salt.states.libcloud_dns.html
name
withdomain
and addedtype: master
I've got this:This is because
create_zone
callsconn.create_record
instead ofconn.create_zone
: https://github.com/saltstack/salt/blob/develop/salt/modules/libcloud_dns.py#L213Unit tests are nominally present, but unable to catch this, because they just test for
True
and don't care which function was actually invoked: https://github.com/saltstack/salt/blob/develop/tests/unit/states/test_libcloud_dns.py#L123test=True
:Maybe it indeed was a bad idea to rename
name
todomain
?test=True
and actually created the zone on step (4). This could lead to really bad production incidents, and violates one of the biggest expectations about Salt https://docs.saltstack.com/en/latest/ref/states/testing.htmlSomething like this (pretty lame and with some false positives, but anyway) could flag new state modules which do not handle the
test=True
for code review:and I'm about to add some DNS records from the example https://docs.saltstack.com/en/latest/ref/states/all/salt.states.libcloud_dns.html
Guess what?
Nope!
Maybe the state module was written against some old version of
apache-libcloud
, but the current iterface wants anextra
dictionary which holds all the parameters, includingttl
:https://github.com/apache/libcloud/blob/trunk/libcloud/dns/drivers/gandi.py#L232 https://github.com/apache/libcloud/blob/trunk/libcloud/dns/drivers/route53.py#L190 https://github.com/apache/libcloud/blob/trunk/libcloud/dns/drivers/linode.py#L162
At this point I stopped going down this rabbit hole and decided to submit an issue. Sorry for being grumpy, but this is just bugs upon bugs and is really frustrating.
python-3.6.6
,salt-2018.3.3
,apache-libcloud-2.3.0
CC @tonybaloney