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

change data type + default value of updatefreq to Numeric #14

Merged
merged 2 commits into from
Jan 4, 2023

Conversation

fraenki
Copy link
Contributor

@fraenki fraenki commented Jan 4, 2023

Currently the module supports setting the alias attribute "updatefreq" to an empty string. However, this causes Puppet to alter the affected alias on every Puppet run:

Notice: /Stage[main]/Opnsense/Opnsense_firewall_alias[test_alias]/updatefreq: updatefreq changed 0.000000 to '' (corrective)

This happens because OPNsense expects only numeric values for "updatefreq":

https://github.com/opnsense/core/blob/6ca0e5b58f2049d09d9776fbce31cfd785dfeea9/src/opnsense/mvc/app/models/OPNsense/Firewall/Alias.xml#L70-L72

Empty strings are still accepted, but are internally converted to 0 by OPNsense.

So in order to solve this, this PR does two things:

  • change the accepted data type from "Float" to "Numeric" (to align it with OPNsense' data type)
  • change the default value from an empty string to 0 (the default value that is internally enforced by OPNsense)

This change is fully backwards-compatible, because it just sets the same value that is already enforced by OPNsense. However, an alias that was previously relying on the old default value (empty string) will see the following change on the next Puppet run:

Notice: /Stage[main]/Opnsense/Opnsense_firewall_alias[test_alias]/updatefreq: updatefreq changed '' to 0

All following Puppet runs will be completely silent. :)

(Side note: This behaviour seems to be newish, it probably started in OPNsense 22.7 and I'd assume that it is related to the phalcon/PHP upgrades. In older versions of OPNsense an emptry string for "updatefreq" was actually written to config.xml and not replaced with a 0.)

@fraenki fraenki force-pushed the fix_updatefreq branch 5 times, most recently from 8c72859 to e23c570 Compare January 4, 2023 13:02
@codecov
Copy link

codecov bot commented Jan 4, 2023

Codecov Report

Base: 100.00% // Head: 100.00% // No change to project coverage 👍

Coverage data is based on head (bf9a164) compared to base (a95f002).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff            @@
##              main       #14   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           17        17           
  Lines          511       511           
=========================================
  Hits           511       511           
Impacted Files Coverage Δ
lib/puppet/type/opnsense_firewall_alias.rb 100.00% <ø> (ø)
...opnsense_firewall_alias/opnsense_firewall_alias.rb 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@fraenki
Copy link
Contributor Author

fraenki commented Jan 4, 2023

The initial acceptance test failure is unrelated to this PR:

Failure/Error: vmhostname = LitmusHelper.instance.run_shell('hostname').stdout.strip
[...]
"stderr"=>"bash: hostname: command not found\n"

https://github.com/andeman/puppet-opnsense/actions/runs/3837663303/jobs/6533223983

This seems to be related to the (dead) CentOS 8 test container. I've modified the acceptance test to use the (maintained) Rocky 8 container instead (which includes the hostname command). Rocky aims to be binary-compatible with RHEL 8, so this should not make any difference.

@andeman Feel free to review/merge :)

In OPNsense "updatefreq" expects only numeric values. Empty strings are
still accepted, but will automatically be converted to '0'. When using
empty strings for "updatefreq", Puppet would try to change the value on
every Puppet run.
@andreas-stuerz andreas-stuerz added the bugfix Changelog Section: Fixes label Jan 4, 2023
@andreas-stuerz andreas-stuerz merged commit e5c6f05 into andreas-stuerz:main Jan 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Changelog Section: Fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants