Skip to content

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 requested a review from nemesifier February 15, 2021 09:53
atb00ker
atb00ker previously approved these changes Mar 9, 2021
@atb00ker
Copy link
Member

atb00ker commented Mar 9, 2021

Looks good, but deferring to Fed.

@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 force-pushed the remove-mandatory-items branch from cd0d762 to 0968e43 Compare November 29, 2022 20:44
@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
@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.

@nemesifier nemesifier deleted the remove-mandatory-items branch March 19, 2025 18:54
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.

5 participants