Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[bug] Fix switch vlan ordering #104 #149

Merged
merged 1 commit into from
Apr 11, 2020
Merged

Conversation

atb00ker
Copy link
Member

@atb00ker atb00ker commented Apr 4, 2020

I found that the issue #104 is because the order of the list items is causing problems, switch_vlan is defined and processed before switch which is causing KeyError as switch itself is not existing at the time switch_vlan is being processed.

This pull request solves the problem by reordering the intermediate list.

Closes #104

Please let me know if there is a better way to solve this issue!

@atb00ker atb00ker changed the title [bug] Fix switch vlan ordering #104 [WIP] [bug] Fix switch vlan ordering #104 Apr 4, 2020
@atb00ker atb00ker changed the title [WIP] [bug] Fix switch vlan ordering #104 [bug] Fix switch vlan ordering #104 Apr 4, 2020
@coveralls
Copy link

Coverage Status

Coverage increased (+0.0004%) to 99.931% when pulling 772cec1 on atb00ker:issues/104 into 90ab88b on openwisp:master.

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

Thanks for the fix Ajay, it works!

From a purely coding related point of view, it is not 100% correct, see my comment below, but I'm going to fix this now in order to speed it up.

PS: here's the improvement: 5182887

@@ -109,6 +109,19 @@ def to_netjson(self, remove_block=True):
# return result, expects dict
return result

def clean_intermediate_data(self, intermediate_data):
Copy link
Member

Choose a reason for hiding this comment

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

you're adding a fix for the OpenWrt backend in the Base converter.

Base classes are supposed to be generic.

I am going to handle this issue on my own. Thanks for the fix.

Copy link
Member Author

Choose a reason for hiding this comment

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

That was actually a conscious decision, I thought other backends like (openwisp 1.X) will need this change as well, however, I have not worked with any other backend, so I might have misunderstood it incorrectly.
Thanks for the information! 😄

Copy link
Member

Choose a reason for hiding this comment

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

OpenWrt derivatives will have to install OpenWrt base classes.

Don't forget it's possible to implement backends for non-openwrt systems.
We implemented two experimental backends in the past:

The goal of OpenWISP is to support multiple systems.

@nemesifier nemesifier merged commit 772cec1 into openwisp:master Apr 11, 2020
@atb00ker atb00ker deleted the issues/104 branch July 16, 2020 10:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

[backward-conversion] fails when parsing LEDE config.tgz
3 participants