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

Removing check for chkconfig #7861

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

smalenfant
Copy link
Contributor

Closes: #7858

Which Traffic Control components are affected by this PR?

  • Traffic Control Cache Config (t3c, formerly ORT)

What is the best way to verify this PR?

If this is a bugfix, which Traffic Control versions contained the bug?

  • 6.0.x through master

PR submission checklist

No test is needed. No documentation is needed.

Copy link

codecov bot commented Nov 8, 2023

Codecov Report

Merging #7861 (64dbc52) into master (23e9248) will decrease coverage by 2.90%.
Report is 39 commits behind head on master.
The diff coverage is 25.00%.

@@             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     
Flag Coverage Δ
golib_unit 53.86% <33.33%> (+0.28%) ⬆️
grove_unit 12.02% <ø> (ø)
t3c_unit 5.91% <0.00%> (+<0.01%) ⬆️
traffic_monitor_unit 26.44% <ø> (ø)
traffic_ops_unit 21.68% <ø> (ø)
traffic_portal_v2 ?
traffic_stats_unit 10.78% <ø> (ø)
unit_tests 25.81% <25.00%> (-3.39%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
cache-config/t3c-apply/torequest/cmd.go 0.00% <ø> (ø)
cache-config/t3c-apply/torequest/torequest.go 5.65% <ø> (ø)
cache-config/t3c-apply/util/gitutil.go 0.00% <ø> (ø)
cache-config/t3c-apply/util/util.go 0.00% <ø> (ø)
cache-config/t3c-check-refs/config/config.go 0.00% <ø> (ø)
cache-config/t3c-check-refs/t3c-check-refs.go 1.78% <ø> (ø)
cache-config/t3c-generate/cfgfile/all.go 51.38% <ø> (ø)
cache-config/t3c-generate/cfgfile/routing.go 84.00% <ø> (ø)
cache-config/t3c-generate/cfgfile/sslkeys.go 0.00% <ø> (ø)
cache-config/t3c-generate/cfgfile/varnish.go 0.00% <ø> (ø)
... and 64 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!

@ocket8888 ocket8888 added cache-config Cache config generation medium impact impacts a significant portion of a CDN, or has the potential to do so bug something isn't working as intended labels Nov 10, 2023
}
if !isCommandAvailable(Chkconfig) {
} else {
Copy link
Contributor

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
	}

Copy link
Contributor

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).

Copy link
Contributor

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something isn't working as intended cache-config Cache config generation medium impact impacts a significant portion of a CDN, or has the potential to do so
Projects
None yet
Development

Successfully merging this pull request may close these issues.

t3c: Can't detect SystemD in Rocky Linux 9
3 participants