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

[feature] Added "auth_nocache" option to OpenVPN backend #215

Merged
merged 1 commit into from
Mar 7, 2022

Conversation

aagman945
Copy link
Contributor

@aagman945 aagman945 commented Feb 23, 2022

Added auth_nocache to OpenVPN

Closes #177

@pandafy pandafy self-requested a review February 23, 2022 15:32
@pandafy pandafy added this to In progress in OpenWISP Priorities for next releases via automation Feb 23, 2022
@pandafy pandafy moved this from In progress to Ready for review/testing in OpenWISP Priorities for next releases Feb 23, 2022
@coveralls
Copy link

coveralls commented Feb 23, 2022

Coverage Status

Coverage remained the same at 99.94% when pulling e587287 on aagman945:issue#177 into 47b4435 on openwisp:master.

Copy link
Member

@pandafy pandafy left a comment

Choose a reason for hiding this comment

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

@aagman945 this PR will need some more work before merging.

We will need to update the auto_client function of OpenVpn class to add auth_nocache field.

@classmethod
def auto_client(
cls,
host,
server,
ca_path=None,
ca_contents=None,
cert_path=None,
cert_contents=None,
key_path=None,
key_contents=None,
):

Adding auth_nocache in the copy_keys list should do it.

We will also need to update tests after
making this change. In the tests, set the value of auth_nocache to True.

The documentation needs to be updated as well to reflect addition of this field.

netjsonconfig/backends/openvpn/schema.py Outdated Show resolved Hide resolved
netjsonconfig/backends/openvpn/schema.py Outdated Show resolved Hide resolved
OpenWISP Priorities for next releases automation moved this from Ready for review/testing to In progress Feb 23, 2022
@aagman945
Copy link
Contributor Author

@pandafy should I update all the tests, or just test_server_mode & test_client_mode .

@pandafy pandafy self-requested a review February 24, 2022 10:00
@pandafy pandafy moved this from In progress to Ready for review/testing in OpenWISP Priorities for next releases Mar 3, 2022
@pandafy pandafy changed the title [feature] Added OpenVPN auth_nocache [feature] Added "auth_nocache" option to OpenVPN backend Mar 3, 2022
Copy link
Member

@pandafy pandafy left a comment

Choose a reason for hiding this comment

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

Good work @aagman945, everything looks good to me. I have tested this with openwisp-controller as well.
The only problem I see is with the commit message. The commit message does not reference the issue. Read the commit style guideline here https://openwisp.io/docs/developer/contributing.html#how-to-commit-your-changes-properly

Also, can you squash you commits and rebase you PR on the latest master?

OpenWISP Priorities for next releases automation moved this from Ready for review/testing to In progress Mar 3, 2022
@aagman945 aagman945 requested a review from pandafy March 4, 2022 05:39
@aagman945
Copy link
Contributor Author

Good work @aagman945, everything looks good to me. I have tested this with openwisp-controller as well. The only problem I see is with the commit message. The commit message does not reference the issue. Read the commit style guideline here https://openwisp.io/docs/developer/contributing.html#how-to-commit-your-changes-properly

Also, can you squash you commits and rebase you PR on the latest master?
@pandafy Sorry ,My bad
Done with the required changes

Copy link
Member

@pandafy pandafy left a comment

Choose a reason for hiding this comment

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

@aagman945 the build is failing!

Copy link
Member

@pandafy pandafy left a comment

Choose a reason for hiding this comment

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

LGTM! 👍🏼

@aagman945 the first word of the commit description should be capitalised ("Closes" and not "closes"). This time, we will update the description while merging. But if you are eager to continue contributing to OpenWISP, I ask you to please take care of these small details from next time.

Thanks for contributing!

Deferring merge to @nemesisdesign 😄

OpenWISP Priorities for next releases automation moved this from In progress to Reviewer approved Mar 7, 2022
@pandafy pandafy requested a review from nemesifier March 7, 2022 10:32
@aagman945
Copy link
Contributor Author

LGTM! 👍🏼

@aagman945 the first word of the commit description should be capitalised ("Closes" and not "closes"). This time, we will update the description while merging. But if you are eager to continue contributing to OpenWISP, I ask you to please take care of these small details from next time.

Thanks for contributing!

Deferring merge to @nemesisdesign smile

I'll take care from next time.

@nemesifier nemesifier merged commit b06331a into openwisp:master Mar 7, 2022
OpenWISP Priorities for next releases automation moved this from Reviewer approved to Done Mar 7, 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.

[feature] Add support for OpenVPN auth_nocache
4 participants