-
Notifications
You must be signed in to change notification settings - Fork 936
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 path for new opnsense version #3367
base: master
Are you sure you want to change the base?
Conversation
From 24.x the file version doesn't exist anymore
WalkthroughThe pull request updates the command used to retrieve the version of OPNsense within the Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lib/oxidized/model/opnsense.rb (1)
Line range hint
11-14
: Update the comment to reflect the new pathThe existing comment mentions the old path
/usr/local/opnsense/version/opnsense
but should be updated to reflect the new path.Apply this diff to update the comment:
# This gets the version using the opnsense-version command, or from the - # /usr/local/opnsense/version/opnsense file for earlier versions of OPNsense + # /usr/local/opnsense/version/base file for OPNsense 24.x and later, or from + # /usr/local/opnsense/version/opnsense file for earlier versions of OPNsense # that lack the opnsense-version command. Newer versions of OPNsense no longer # store the version information in this file, so both versions have to be # supported here for now.Also applies to: 21-21
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/oxidized/model/opnsense.rb
(1 hunks)
🔇 Additional comments (1)
lib/oxidized/model/opnsense.rb (1)
21-21
: Verify the new path in OPNsense 24.xThe change from
/usr/local/opnsense/version/opnsense
to/usr/local/opnsense/version/base
aligns with the PR objective to support OPNsense 24.x. However, we should verify this is the correct path.Run this script to search for any related discussions or documentation about this path change:
✅ Verification successful
Update comments to reflect the new version file path
The change to
/usr/local/opnsense/version/base
is aligned with the code comments indicating that newer OPNsense versions handle version information differently. Since this is a fallback mechanism whenopnsense-version
fails, the change is appropriate. Consider updating the existing comments to document the new path for clarity.• Update the comments to mention that OPNsense 24.x uses
/usr/local/opnsense/version/base
for version information🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for discussions about the version file path change rg -i "opnsense.*version.*base" rg -i "version/base.*24"Length of output: 187
Script:
#!/bin/bash # Search for test files and documentation related to OPNsense fd -t f -e rb opnsense fd -t f -e md opnsense # Search for version-related patterns rg -i "version.*file.*opnsense" -A 2 rg -i "opnsense.*version.*file" -A 2 # Search for any migration or compatibility notes rg -i "migration.*opnsense" rg -i "compatibility.*opnsense"Length of output: 880
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
From the model code, I read that newer versions of opnsense use |
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #3367 +/- ##
==========================================
- Coverage 71.06% 70.05% -1.02%
==========================================
Files 82 71 -11
Lines 3328 3299 -29
==========================================
- Hits 2365 2311 -54
- Misses 963 988 +25 ☔ View full report in Codecov by Sentry. |
Yes it exist but I think it is not in path or something like that. When oxiddized get config i have this output:
|
From 24.x the file opnsense doesn't exist anymore
Pre-Request Checklist
rubocop --auto-correct
)rake test
)Description
Summary by CodeRabbit