-
Notifications
You must be signed in to change notification settings - Fork 346
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
Removing check for chkconfig #7861
base: master
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## master #7861 +/- ##
============================================
- Coverage 31.87% 28.97% -2.90%
Complexity 98 98
============================================
Files 717 602 -115
Lines 82756 77414 -5342
Branches 970 90 -880
============================================
- Hits 26377 22432 -3945
+ Misses 54218 52888 -1330
+ Partials 2161 2094 -67
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 150 files with indirect coverage changes 📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today! |
} | ||
if !isCommandAvailable(Chkconfig) { | ||
} else { |
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.
Since systemV needs chkconfig maybe we should do this.
if _svcManager == SystemV && !isCommandAvailable(Chkconfig) {
return Unknown
}
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.
Do we support System V? That's older than me. Far as I'm aware we only support RHEL 7, 8 & 9 (and only some of those distros and only to varying degrees, for whatever reason).
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.
That is true, I think you have to go back to RHEL 6 for System V. That said, we could probably remove the System V check too and just return unknown if it's not System D.
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.
Does that sound good to you, @smalenfant?
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.
Centos/Rocky 7,8 and 9 support chkconfig. Just depends if it's installed or not. That's the wrong way to detect which service manager is installed. This is really up to the software used (ATS) and how it's been packaged. I think we should remove these checks entirely as they seem invalid and just use systemctl by default.
Using systemctl for SystemV type works anyway. By removing, we just don't support Centos 6 anymore
Closes: #7858
Which Traffic Control components are affected by this PR?
t3c
, formerly ORT)What is the best way to verify this PR?
If this is a bugfix, which Traffic Control versions contained the bug?
PR submission checklist
No test is needed. No documentation is needed.