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] Remove mandatory items #183

Closed
wants to merge 1 commit into from
Closed

Conversation

okraits
Copy link
Member

@okraits okraits commented Feb 12, 2021

In certain situations, especially when modifying an existing openvpn config, it shouldn't be necessary to provide certain items as they might already exist in the openvpn config.

@coveralls
Copy link

coveralls commented Feb 12, 2021

Coverage Status

Coverage remained the same at 99.635% when pulling 0968e43 on remove-mandatory-items into 599d40b on master.

@okraits okraits added this to Ready for review/testing in OpenWISP Priorities for next releases Feb 12, 2021
atb00ker
atb00ker previously approved these changes Mar 9, 2021
OpenWISP Priorities for next releases automation moved this from Ready for review/testing to Reviewer approved Mar 9, 2021
@atb00ker
Copy link
Member

atb00ker commented Mar 9, 2021

Looks good, but deferring to Fed.

@nemesifier nemesifier moved this from Reviewer approved to Ready for review/testing in OpenWISP Priorities for next releases Jan 25, 2022
OpenWISP Priorities for next releases automation moved this from Ready for review/testing to In progress May 20, 2022
@okraits
Copy link
Member Author

okraits commented May 20, 2022

@nemesisdesign I rebased this to current master. Please let me know if there's anything which keeps this from getting merged.

@okraits okraits moved this from In progress to Ready for review/testing in OpenWISP Priorities for next releases May 20, 2022
@okraits
Copy link
Member Author

okraits commented Nov 29, 2022

@nemesisdesign What about this one? This is in the queue for one and a half year...

@nemesifier
Copy link
Member

@nemesisdesign What about this one? This is in the queue for one and a half year...

Sorry for having forgotten this.
This can cause problems to existing systems which rely on this check, I was looking for an alternative way and I believe applying this change can be applied with monkey patching in your own system, I have done similar changes in a few deployments.

I want to bring up again the subject of #120 to add easy ways to enable changes to the backends, changes could mean enabling optional schema sections for openwrt packages that are not installed by default or register schema changes like this.

@okraits
Copy link
Member Author

okraits commented Nov 29, 2022

I'm already maintaining a patch for this for one and a half year and we use it since then without problems - that's why I wanted to get this upstream and get rid of the patch ;-)

@nemesifier
Copy link
Member

nemesifier commented Nov 30, 2022

@okraits instead of patching the library, you can apply this patch in your django project, import the module in the settings and update the schema dictionary there, you will not need to maintain a fork of netjsonconfig, that's how I have been doing it and it's working pretty well so I recommend it to you too.

@nemesifier nemesifier closed this Nov 30, 2022
OpenWISP Priorities for next releases automation moved this from Ready for review/testing to Done Nov 30, 2022
@okraits
Copy link
Member Author

okraits commented Nov 30, 2022

@nemesisdesign Why did you close the PR? Don't you want to fix this issue?

@nemesifier
Copy link
Member

@okraits as I said, this cannot be accepted as it can create issues on systems which rely on these checks. Instead of patching the library, you can apply this patch in your django project, import the module in the settings and update the schema dictionary there, you will not need to maintain a fork of netjsonconfig, that's how I have been doing it and it's working pretty well so I recommend it to you too. If I am missing something let me know and give me more details please.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants