Adding missing idempotence support in load balancer #45548
Conversation
zikalino
force-pushed
the
lb-fixing-idempotency
branch
from
c7aa715
to
f9745d2
Compare
ansibot
added
affects_2.8
ansibot
added
needs_revision
ansibot
added
test
|
CI failure in integration tests: https://app.shippable.com/github/ansible/ansible/runs/83704/85/tests |
mattclay
added
the
ci_verified
|
The test |
ansibot
removed
the
needs_triage
zikalino
changed the title
ansibot
removed
the
ci_verified
|
The test |
|
The test |
|
The test |
zikalino
force-pushed
the
lb-fixing-idempotency
branch
from
6844127
to
5e1b4b7
Compare
ansibot
removed
stale_ci
| if not compare(a.get(k, None), b.get(k, None), t[k]): | ||
| return False | ||
| if not compare(a.get(k, None), b.get(k, None), t[k]): | ||
| return False |
There was a problem hiding this comment.
line 875, what if a==none and b==none, the compare return true, but it should be false.
There was a problem hiding this comment.
line 861, if s is not None, be specific s=="index"
There was a problem hiding this comment.
line 858-860, no matter what t is, how many properties in it, if one of properties =="index", s will be "index". sorry I didn't get it.
There was a problem hiding this comment.
line 887, default comparison, not considering number
|
The test The test The test The test The test The test The test The test The test The test The test The test The test The test The test The test The test The test The test The test |
|
The test The test |
| @@ -784,7 +790,7 @@ def get_load_balancer(self): | |||
| """Get a load balancer""" | |||
| self.log('Fetching loadbalancer {0}'.format(self.name)) | |||
| try: | |||
| return self.network_client.load_balancers.get(self.resource_group, self.name) | |||
| return self.network_client.load_balancers.get(self.resource_group, self.name).as_dict() | |||
There was a problem hiding this comment.
i'm not sure as_dict() whether works on load balancer, but it doesn't not work on webapp objects
There was a problem hiding this comment.
what happens in webapp?
|
|
||
| return result | ||
| def default_compare(new, old, path): | ||
| if new is None: |
There was a problem hiding this comment.
you mean if old is not none?
at this point I am just assuming that if parameter is not specified (is None), then we don't do comparison. I am not sure whether it's right or wrong default behavior in idempotency check.
There was a problem hiding this comment.
then why check new not None?
There was a problem hiding this comment.
in this case we should just remove this check.... and then it will fall under else.
There was a problem hiding this comment.
actually i would suggest keep none check inside this function. as a function, you cannot have assumption on what will be passed in. it's hard to rely on caller to do none check.
There was a problem hiding this comment.
well, I just removed it, if it's None it will fall under else, and there will be just == comparison, which will return True if old is also None, or False if old is something else.
| if new is None: | ||
| return True | ||
| elif isinstance(new, dict): | ||
| if not isinstance(old, dict): |
There was a problem hiding this comment.
if new is dict, and old is None, that means previously user didn't set some properties(None), now, user want to update it(dict), why return False?
There was a problem hiding this comment.
so that correct, returning False will trigger update.
There was a problem hiding this comment.
ok. so true if compare equals, false if not. Seems like equal?
| return False | ||
| return True | ||
| elif isinstance(new, list): | ||
| if not isinstance(old, list) or len(new) != len(old): |
There was a problem hiding this comment.
same as above, if old is None. such as previous rules is None
There was a problem hiding this comment.
so we will return False and that will trigger update.
|
the compare method is ok for this module specific. while if want to make it general, need rethink the interface and compare way, actually it's equal. equal need 2 params, and third one is optional, such as sort order, comparator etc. suggest to compare by keys matching one by one, instead of requires a dict of k, with some not-so-straight-forward values: index, default etc. |
|
@yungezz I am going to propose extended method. it will be very simple. |
SUMMARY
Idempotence is not supported at all.
Load balancer can't be updated at the moment
ISSUE TYPE
COMPONENT NAME
azure_rm_loadbalancer
ANSIBLE VERSION
2.7
ADDITIONAL INFORMATION