-
Notifications
You must be signed in to change notification settings - Fork 23
address couple of issues with the parsing #79
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
base: main
Are you sure you want to change the base?
Conversation
addresses AthennaMind#70 - while here also look at Priority being int not string Signed-off-by: Felix Kronlage-Dammers <[email protected]>
|
This was changed in recent versions of OPNSense, right? Is it backwards compatible? If not, we will proceed anyways, we should just document this in the Release. |
good point. will check and see to maybe add some case'ing. Will come back with this. |
|
yeah. with older version this breaks. |
|
@fkr Thanks for checking this out! I'm not sure we should introduce such complexity. OPNSense evolves overtime, so our exporter does. I think users using older versions of OPNSense should use older versions of the exporter. We can just document which version of the exporter is compatible against which version of OPNSense. Also state in the release page that this is breaking change for older versions. Maybe we can hear other opinions from the community as well? |
|
@ihatemodels indeed - I concur with not making it overly complex. The only reason it came to mind is: the Business Edition of OPNsense is typically one major behind. (25.7.x vs. 25.4.x) . Are there others here interested in having the current version as well as BE working with the exporter? (eg. same as @ihatemodels mentioned: what does the community say? :) |
|
Latest = latest, if you run something older use an older exporter |
|
In my opinion, the latest exporter should support the latest opnsense version. So, I totally agree with @Finkregh and @ihatemodels. If you're running the BE version, use an older version that works with it. |
|
Thanks for adding feedback @stefanDeveloper and @Finkregh. @ihatemodels I will adjust this PR to add something to the docs. |
addresses #70 - while here also look at Priority being int not string
Description
Attempt at fixing the issue raised in #70
The code tries to parse String to bool, while already a bool is technically present.
Fixes # (issue)
#70
Type of change
Please delete options that are not relevant.