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

Conf name #108

Merged
merged 8 commits into from Mar 7, 2018
Merged

Conf name #108

merged 8 commits into from Mar 7, 2018

Conversation

jreidinger
Copy link
Member

No description provided.

Copy link
Contributor

@imobachgs imobachgs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a minor comment. Otherwise, it looks good.

@@ -35,6 +35,9 @@ class NtpClientClass < Module

NTP_FILE = "/etc/chrony.conf".freeze

# The cron file name for the synchronization.
CRON_FILE = "/etc/cron.d/suse-ntp_synchronize"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just add the call to #freeze.

@lslezak lslezak changed the title Conf na,e Conf name Mar 7, 2018
@lslezak
Copy link
Member

lslezak commented Mar 7, 2018

👮 Rubocop complains...

@jreidinger
Copy link
Member Author

manual testing of RPM upgrade works well, so I will pleasure rubocop and add changes

Copy link
Contributor

@imobachgs imobachgs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a comment. Otherwise, it looks good.

(bsc#1079122)
- convert old file to new one during upgrade, ensure that ntpd is
replaced by chrony (FATE#323432)
- ghost that config file, so it is easy to find who own it
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"who own it" or "which package owns it" ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or "which package belongs to" ?

@coveralls
Copy link

coveralls commented Mar 7, 2018

Coverage Status

Coverage remained the same at 40.0% when pulling b0b7c23 on conf_na,e into 435e222 on master.

Copy link
Member

@lslezak lslezak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Command hint

"-*/#{@sync_interval} * * * * root /usr/sbin/chronyd -q &>/dev/null\n"
)
else
SCR.Execute(
path(".target.bash"),
"test -e #{@cron_file} && rm #{@cron_file};"
"test -e #{CRON_FILE} && rm #{CRON_FILE};"
Copy link
Member

@lslezak lslezak Mar 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NP: there is a trailing ; and maybe simple rm -f #{CRON_FILE} (without the test) would be better (even could be replaced by native Ruby)

Copy link
Member

@lslezak lslezak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jreidinger jreidinger merged commit fa99807 into master Mar 7, 2018
@jreidinger jreidinger deleted the conf_na,e branch March 7, 2018 11:22
@@ -812,13 +812,13 @@ def update_cron_settings
if @synchronize_time
SCR.Write(
path(".target.string"),
@cron_file,
CRON_FILE,
Copy link
Member

@kobliha kobliha Mar 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just an idea, ... but probably too late: Could we have a comment here (in file) that this is created by us and that the user should run YaST to re/configure it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, such comment probably makes good sense. This one is already merged, but I can open new one.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants