Added option 'exclude_disabled' to filter disabled records #24
Conversation
Added defaults sorting for predictable value for weighted CNAME records
There was a problem hiding this comment.
Comments inline. At a high level this seems to be changing multiple things in one PR. The version bit shouldn't happen here. The defaults change seems unrelated to exclude_disabled and thus should happen in its own PR along with related tests.
This PR can be pared back \to just do the exclude_disabled bits.
| @@ -15,7 +15,7 @@ | |||
| from octodns.record import GeoCodes, Record | |||
|
|
|||
| # TODO: remove __VERSION__ with the next major version release | |||
| __version__ = __VERSION__ = '0.0.2' | |||
| __version__ = __VERSION__ = '0.0.3' | |||
There was a problem hiding this comment.
Version bumps happen in dedicated release PRs.
| @@ -219,6 +220,7 @@ def _build_pools(self, record, default_pool_name, value_transform_fn): | |||
|
|
|||
| pools[geo_sets[geo_set]]["values"].append(value) | |||
|
|
|||
| defaults.sort() | |||
There was a problem hiding this comment.
Probably best to include a comment on this change as to why it's needed/what it fixes.
| _result_resource_record = [] | ||
| for resource_record in record["resource_records"]: | ||
| if resource_record.get("enabled", True): | ||
| _result_resource_record.append(resource_record) |
There was a problem hiding this comment.
What happens if all the rr's are eanbled=False? Looks like record["resource_records"] would be []. Is that valid?
| @@ -39,7 +39,7 @@ | |||
| - pool: weight | |||
| ttl: 300 | |||
| type: CNAME | |||
| value: cl-dabdb3fc.edgecdn.ru. | |||
| value: cl-dabdb1fc.edgecdn.ru. | |||
There was a problem hiding this comment.
What's this change for? I don't see any corresponding changes in the tests below.
Firstly, sorting has been added for
defaults. This solves the problem when two different plugins with support weighted records choose different value for the weighted CNAME record.Second. I added an option
exclude_disabledto filter rrset that haveebabled: false