Skip to content

Handle invalid type for excluded_interfaces#5986

Merged
therve merged 2 commits into
masterfrom
therve/network-excluded-none
Mar 6, 2020
Merged

Handle invalid type for excluded_interfaces#5986
therve merged 2 commits into
masterfrom
therve/network-excluded-none

Conversation

@therve

@therve therve commented Mar 6, 2020

Copy link
Copy Markdown
Contributor

Let's raise a proper error when the configuration is incorrect.

Let's raise a proper error when the configuration is incorrect.

@florimondmanca florimondmanca left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice lil' improvement 👍

Comment thread network/tests/test_network.py Outdated
instance = {}

self._excluded_ifaces = instance.get('excluded_interfaces', [])
if not isinstance(self._excluded_ifaces, list):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this happen at init time?

@florimondmanca florimondmanca Mar 6, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally… Looks like the Network check is pretty old school as it actually uses the instance passed to .check() instead of self.instance… Looks like a candidate for a refactor IMO, though this PR is consistent with the rest of the config handling done here so far.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah we can do that later.

@albertvaka albertvaka left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this needs agent-platform's approval...

@therve
therve merged commit 74d07f8 into master Mar 6, 2020
@therve
therve deleted the therve/network-excluded-none branch March 6, 2020 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants