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

Add autoyast test for yast2-ntp-client systemd timers #9988

Merged
merged 1 commit into from
Apr 20, 2020

Conversation

sofiasyria
Copy link
Contributor

@sofiasyria sofiasyria commented Apr 7, 2020

yast2-ntp-client is using systemd timers instead of cron, when configured to perform one time synchronization. This test verifies that particular function works as configured.

@sofiasyria sofiasyria force-pushed the ac61946 branch 2 times, most recently from c4a4adb to 1090638 Compare April 7, 2020 11:25
@sofiasyria sofiasyria changed the title Add autoyast test for yast2-ntp-client systemd timers [WIP]Add autoyast test for yast2-ntp-client systemd timers Apr 7, 2020
tests/console/verify_systemd_timesync.pm Outdated Show resolved Hide resolved
tests/console/verify_systemd_timesync.pm Outdated Show resolved Hide resolved
tests/console/verify_systemd_timesync.pm Outdated Show resolved Hide resolved
tests/console/verify_systemd_timesync.pm Outdated Show resolved Hide resolved
@sofiasyria sofiasyria force-pushed the ac61946 branch 2 times, most recently from c80d475 to db3b454 Compare April 8, 2020 16:33
tests/console/verify_systemd_timesync.pm Outdated Show resolved Hide resolved
tests/console/verify_systemd_timesync.pm Outdated Show resolved Hide resolved
tests/console/verify_systemd_timesync.pm Outdated Show resolved Hide resolved
@sofiasyria sofiasyria force-pushed the ac61946 branch 3 times, most recently from 605b0ba to cb0fa59 Compare April 9, 2020 20:33
tests/console/verify_systemd_timesync.pm Outdated Show resolved Hide resolved
tests/console/verify_systemd_timesync.pm Outdated Show resolved Hide resolved
tests/console/verify_systemd_timesync.pm Outdated Show resolved Hide resolved
tests/console/verify_systemd_timesync.pm Outdated Show resolved Hide resolved
tests/console/verify_systemd_timesync.pm Outdated Show resolved Hide resolved
tests/console/verify_systemd_timesync.pm Outdated Show resolved Hide resolved
@sofiasyria sofiasyria force-pushed the ac61946 branch 3 times, most recently from 03ee938 to a9ddb1e Compare April 14, 2020 12:31
@sofiasyria sofiasyria force-pushed the ac61946 branch 2 times, most recently from 095f947 to 46811df Compare April 14, 2020 13:16
@sofiasyria sofiasyria changed the title [WIP]Add autoyast test for yast2-ntp-client systemd timers Add autoyast test for yast2-ntp-client systemd timers Apr 14, 2020
tests/console/verify_systemd_timesync.pm Outdated Show resolved Hide resolved
tests/console/verify_systemd_timesync.pm Outdated Show resolved Hide resolved
@sofiasyria sofiasyria force-pushed the ac61946 branch 5 times, most recently from cb7b1a6 to f0ce9a3 Compare April 15, 2020 15:10
Copy link
Contributor

@b10n1k b10n1k left a comment

Choose a reason for hiding this comment

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

Some small changes and some ideas. Enjoy :)

tests/console/verify_systemd_timesync.pm Outdated Show resolved Hide resolved
tests/console/verify_systemd_timesync.pm Outdated Show resolved Hide resolved
tests/console/verify_systemd_timesync.pm Outdated Show resolved Hide resolved
tests/console/verify_systemd_timesync.pm Outdated Show resolved Hide resolved
Copy link
Member

@rwx788 rwx788 left a comment

Choose a reason for hiding this comment

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

One more thing not to forget is that we don't validate generated profile (clone installed system and validate ntp settings). Feel free to create follow up ticket for that, so we can merge this PR and proceed in the next iteration.

@sofiasyria sofiasyria force-pushed the ac61946 branch 3 times, most recently from 795d464 to 8df7e8a Compare April 16, 2020 14:26
Copy link
Contributor

@b10n1k b10n1k left a comment

Choose a reason for hiding this comment

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

only a comment for the copyright section and LATM

@sofiasyria sofiasyria force-pushed the ac61946 branch 2 times, most recently from a119647 to 0bfa2df Compare April 17, 2020 15:40
my $expected_server = $test_data->{profile}->{'ntp-client'}->{ntp_servers}->{ntp_server}->{address};
my $chrony_conf = script_output("cat /etc/chrony.conf");
$chrony_conf =~ /(\R|^)pool\s(?<server>(\S+))/;
assert_equals($expected_server, $+{server});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In case of multiple server addresses, only the first one will be taken under consideration.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it is ok for this test and probably not needed a comment as the regex does not have g modifier /regex/g so only one match is expected. Otherwise it would need a loop.

Copy link
Contributor

@jknphy jknphy left a comment

Choose a reason for hiding this comment

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

LGTM besides those minor comments.

record_info("Check server", "Check if the configured time synchronization server is the expected one.");
my $expected_server = $test_data->{profile}->{'ntp-client'}->{ntp_servers}->{ntp_server}->{address};
my $chrony_conf = script_output("cat /etc/chrony.conf");
$chrony_conf =~ /(\R|^)pool\s(?<server>(\S+))/;
Copy link
Contributor

Choose a reason for hiding this comment

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

minor detail: why not just: ^pool\s(?<server>\S+) ? it would detect it even there is not line on top of the searched one and the parenthesis sorrounding \S+ probably not needed due to the named group already created a group.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought that since it is a conf file, somebody who might use the module could have comments above "pool" . In that case, ^pool wouldn't work. I just tried removing the parenthesis from \S+ and it also works. I will change that.

Copy link
Contributor

Choose a reason for hiding this comment

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

comments starts with # in that file? sure it shouldn't match?

my $expected_server = $test_data->{profile}->{'ntp-client'}->{ntp_servers}->{ntp_server}->{address};
my $chrony_conf = script_output("cat /etc/chrony.conf");
$chrony_conf =~ /(\R|^)pool\s(?<server>(\S+))/;
assert_equals($expected_server, $+{server});
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it is ok for this test and probably not needed a comment as the regex does not have g modifier /regex/g so only one match is expected. Otherwise it would need a loop.

@jknphy jknphy self-requested a review April 20, 2020 06:12
ntp_servers:
ntp_server:
address: cz.pool.ntp.org
ntp_sync: 1
Copy link
Member

Choose a reason for hiding this comment

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

As per our discussion, I will create ticket so we could use same format for validation and variables expansion in AY profiles.

@rwx788 rwx788 merged commit ab9ada3 into os-autoinst:master Apr 20, 2020
@sofiasyria sofiasyria deleted the ac61946 branch April 28, 2020 12:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants