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

[openvpn] Add option proto to remote setting #184

Merged
merged 3 commits into from
Mar 14, 2022
Merged

Conversation

okraits
Copy link
Member

@okraits okraits commented Feb 14, 2021

No description provided.

@okraits okraits added this to Ready for review/testing in OpenWISP Priorities for next releases Feb 14, 2021
@coveralls
Copy link

coveralls commented Feb 15, 2021

Coverage Status

Coverage decreased (-0.06%) to 99.709% when pulling 5295e49 on add-proto-to-remote into cfc388a on master.

@okraits
Copy link
Member Author

okraits commented Nov 10, 2021

@nemesisdesign Anything keeping this one from getting merged?

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.

I haven't had time to review this yet, sorry, my efforts right now are mainly focused on other modules, I hope to be able to get back to this in the next months.

@pandafy pandafy self-requested a review February 24, 2022 10:10
OpenWISP Priorities for next releases automation moved this from Ready for review/testing to Reviewer approved Mar 12, 2022
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.

@okraits I implemented a small change in your patch for the following reason:

If the user adds a remote, they would have been asked again which protocol to use, but that information is already been asked above, so the default for the new proto attribute is "auto" which will be ignored and will allow the system to continue as if it was not present, but at the same time it will be possible to override it if needed.

Result in OW-controller:

Screenshot from 2022-03-12 11-15-12

For me it's good to be merged now, let me know what you think.

@okraits
Copy link
Member Author

okraits commented Mar 14, 2022

@nemesisdesign Yes, that sounds like a reasonable solution. Thank you for fixing the issue!

@nemesifier nemesifier merged commit 04e2bf2 into master Mar 14, 2022
OpenWISP Priorities for next releases automation moved this from Reviewer approved to Done Mar 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants