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 integration to new service configuration widget #5641

Merged
merged 1 commit into from
Aug 30, 2018

Conversation

jknphy
Copy link
Contributor

@jknphy jknphy commented Aug 24, 2018

Add integration to new service configuration widget. It includes: yast2_proxy, yast2_samba, yast2_dns_server, yast2_http and yast2_ftp using a mapped version of the user control due to it was necessary to control different behaviors (start, stop, disabled, ...) besides reusing code.

@jknphy jknphy force-pushed the add_service_widget_integration branch from 7c94e49 to 88a3c0d Compare August 27, 2018 13:49
@jknphy jknphy changed the title [WIP] Add service widget y2dns & y2samba [WIP] Add integration to new service configuration widget Aug 27, 2018
@jknphy jknphy changed the title [WIP] Add integration to new service configuration widget Add integration to new service configuration widget Aug 27, 2018
rwx788
rwx788 previously requested changes Aug 27, 2018
my ($self, %args) = @_;
my $after_writing_conf = $args{after_writing_conf} || 'alt-f';
my $after_reboot = $args{after_reboot} || 'alt-a';
my $after_writing_conf_ref = $args{after_writing_conf} || [qw(alt-t keep_current_state)];
Copy link
Member

Choose a reason for hiding this comment

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

I guess it's a bit of overkill to use an array here, with 2 variables it would be more clear and readable. Here we rely on the order, don't check if array element exists at all. I also see that we always pass the same pairs, meaning that we can even map keys in the method itself depending on the action.

Copy link
Contributor Author

@jknphy jknphy Aug 28, 2018

Choose a reason for hiding this comment

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

Are you referring to something like this, right?
my ($after_writing_conf_ref, $after_reboot_ref) = $args;
Good point I can shorten the calls using order, makes sense. For a couple of cases that I am using default values, I can make it mandatory and it is more simple.

The pairs are different, as far as I could find, for example for after_writing_conf: alt-t, alt-f and alt-e. I like the idea to map keys inside the method and not repeat keys but they don't depend on the action, they depends on the caller (and for a while until the shortcut changes again).

The general idea that I tried to implement is that the caller to the subroutine can pass parameters according to what is displayed in the screen, so no need to take care about internals of the subroutine. If he/she sees any shortcut for any field, then use it.

Let me see if with you with your advice I can present it in a different way.

@jknphy jknphy changed the title Add integration to new service configuration widget [WIP] Add integration to new service configuration widget Aug 28, 2018
@jknphy jknphy force-pushed the add_service_widget_integration branch from 88a3c0d to 1d336c8 Compare August 28, 2018 06:58
@jknphy jknphy changed the title [WIP] Add integration to new service configuration widget Add integration to new service configuration widget Aug 28, 2018
@jknphy jknphy force-pushed the add_service_widget_integration branch from 1d336c8 to ff9d5ba Compare August 28, 2018 07:02
@jknphy
Copy link
Contributor Author

jknphy commented Aug 28, 2018

Another smart way to avoid to pass the shortcut and handle them inside would be to match against an array of needles, each one pointing to the same field title with different shortcut, i.e.:
yast2_ncurses_service_after_writing_conf_alt_t.json(png) yast2_ncurses_service_after_writing_conf_alt_f.json(png) yast2_ncurses_service_after_writing_conf_alt_e.json(png)
Therefore we could pass the action, we decide the shortcut according to matching, accessing the field and the perform the action. The only con that I see that I don't know how often wrong needles are created, if the frequency is too much to trust on that, it's better to have the logic in the code hardcoded as it is now, WDYT?

@jknphy jknphy force-pushed the add_service_widget_integration branch from ff9d5ba to 1aacc14 Compare August 28, 2018 14:32
@jknphy
Copy link
Contributor Author

jknphy commented Aug 29, 2018

Discussed possible approaches with @rwx788.
VR with new changes: Tumbleweed-yast2_ncurses

my ($self, %args) = @_;
my $after_writing_conf = $args{after_writing_conf} || 'alt-f';
my $after_reboot = $args{after_reboot} || 'alt-a';
my $after_writing_ref = $args{after_writing};
Copy link
Member

@asmorodskyi asmorodskyi Aug 29, 2018

Choose a reason for hiding this comment

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

before creating this variable had sense because in case nothing passed into a function it will assign default value. but you remove default values so you can :

sub change_service_configuration {
 my ($self, %args) = @_;
 change_service_configuration_step('after_writing_conf', $args{after_writing}) if $args{after_writing};
 change_service_configuration_step('after_reboot',       $args{after_writing})  if $args{after_reboot};
}

but you can say that it is matter of taste and I will not argue :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There were many combinations and the default values didn't really help. For clarity is all passed to the subroutine and if not, means that we want to select specific field to change, as you said, a matter of taste :)

@rwx788 rwx788 dismissed their stale review August 29, 2018 15:51

Changes in the code

@@ -216,8 +219,8 @@ sub run {
# move to Access Control and change something
send_key_until_needlematch 'yast2_proxy_safe_ports_selected', 'tab';
send_key 'alt-w';
wait_still_screen 1;
send_key 'alt-w';
wait_still_screen 1; after_reboot =>
Copy link
Member

Choose a reason for hiding this comment

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

I guess this is unwanted change

@@ -329,7 +341,7 @@ sub run {
record_soft_failure "bsc#1088152";
setup_yast2_ldap_server;
}
setup_samba;
setup_samba($self);
Copy link
Member

Choose a reason for hiding this comment

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

You could use $self->setup_samba here which is more common, but still is a matter of taste.

send_key 'alt-w'; # make sure ftp start-up when booting
assert_screen 'ftp_server_when_booting'; # check service start when booting
assert_screen 'ftp-server'; # check ftp server configuration page
if (is_sle('<15') || is_leap('<15.0')) {
Copy link
Member

Choose a reason for hiding this comment

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

Is is the case that we have different conditions for different YaST modules? I see 3 variations, wonder if it's the case.

Copy link
Contributor Author

@jknphy jknphy Aug 30, 2018

Choose a reason for hiding this comment

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

wrong paste, good catch! We have two cases, is_sle('<15') || is_leap('<15.1') and another one that also include tw, as in samba is not yet for that product.

@jknphy jknphy force-pushed the add_service_widget_integration branch from 1aacc14 to e1131a8 Compare August 30, 2018 05:35
@rwx788 rwx788 merged commit e57fdc6 into os-autoinst:master Aug 30, 2018
@jknphy jknphy deleted the add_service_widget_integration branch September 7, 2018 10:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants