Enable/disable chronyd service: Enable/disable chrony-wait #137
Conversation
src/modules/NtpClient.rb
Outdated
| @@ -594,6 +595,7 @@ def dhcp_ntp_servers | |||
| publish variable: :synchronize_time, type: "boolean" | |||
| publish variable: :sync_interval, type: "integer" | |||
| publish variable: :service_name, type: "string" | |||
| publish variable: :wait_service_name, type: "string" | |||
There was a problem hiding this comment.
do we need to publish this variable to component system? Is it used outside?
| @service_name = "chronyd" | ||
| @wait_service_name = "chrony-wait" |
There was a problem hiding this comment.
can you please add here comment why it is needed, ideally with reference to bugzilla.
test/ntp_client_test.rb
Outdated
| subject.Write | ||
| it "enables and restarts services" do | ||
| allow(subject).to receive(:check_service).and_call_original | ||
| expect(Yast::Service).to receive(:Enable).with(subject.service_name).and_return(true) |
There was a problem hiding this comment.
I think better it to use explicit string. So this way accessor won't be needed.
src/modules/NtpClient.rb
Outdated
| Report.Error(Message.CannotAdjustService(@service_name)) | ||
| elsif !Service.Disable(@wait_service_name) | ||
| Report.Error(Message.CannotAdjustService(@wait_service_name)) | ||
| end |
There was a problem hiding this comment.
this deserves splitting into own method.
|
|
||
| # error report | ||
| Report.Error(Message.CannotAdjustService("NTP")) unless adjusted | ||
| if @run_service |
There was a problem hiding this comment.
this method contains two separate workflows enclosed in two if @run_service branches. Moreover first of them do not contain else part. It can be rewritten to avoid one level of nesting e.g. like
def check_service
if !@run_service
Service.Stop(@service_name)
Service.Stop(@wait_service_name)
return
end
....
end
which seems to be more readable for me.
|
Apart of check_service name which i see a bit misleading it lgtm. So, lets wait in @jreidinger comments. |
|
❌ Public Jenkins job #16 failed |
|
❌ Internal Jenkins job #1 failed |
|
✔️ Internal Jenkins job #2 successfully finished |
No description provided.