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

Use script_retry instead of shell for loops #86

Closed
wants to merge 1 commit into from

Conversation

kalikiana
Copy link
Member

@kalikiana kalikiana force-pushed the use_script_retry_everywhere branch 2 times, most recently from d5347ca to c267ba8 Compare May 11, 2022 15:01
Copy link
Member

@okurz okurz left a comment

Choose a reason for hiding this comment

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

First, why should script_retry be better than the bash-internal loop? Second, where should script_retry come from? I think again you are confusing os-autoinst-distri-openQA and os-autoinst-distri-opensuse. In any case I prefer the bash internal loop to prevent unnecessary scope switching between bash within the SUT and perl code on the outside worker environment

@kalikiana kalikiana force-pushed the use_script_retry_everywhere branch from c267ba8 to 14f5f81 Compare May 11, 2022 15:09
@okurz okurz mentioned this pull request May 11, 2022
script_retry('ping -c1 -W1 machine', retry => 5);

=cut
sub script_retry {
Copy link
Member

Choose a reason for hiding this comment

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

Better don't copy-paste the implementation when we have a better approach in the working already, see os-autoinst/os-autoinst#2004

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't understand your comment given your saying the function worse than what we have.

@kalikiana
Copy link
Member Author

First, why should script_retry be better than the bash-internal loop? Second, where should script_retry come from? I think again you are confusing os-autoinst-distri-openQA and os-autoinst-distri-opensuse. In any case I prefer the bash internal loop to prevent unnecessary scope switching between bash within the SUT and perl code on the outside worker environment

It is based on a suggestion by @grisu48 and yes, I realized after pushing the initial branch that this is a function from distri-opensuse. If you think this is a bad approach, maybe it should be re-considered in os-autoinst-distri-opensuse as well?

@kalikiana kalikiana force-pushed the use_script_retry_everywhere branch from 14f5f81 to a913b17 Compare May 11, 2022 15:15
@okurz
Copy link
Member

okurz commented May 11, 2022

It is based on a suggestion by @grisu48 and yes, I realized after pushing the initial branch that this is a function from distri-opensuse. If you think this is a bad approach, maybe it should be re-considered in os-autoinst-distri-opensuse as well?

Maybe but I leave it to the maintainers of os-autoinst-distri-opensuse to decide that. For this test distribution here I see no benefit

@Martchus
Copy link
Contributor

In any case I prefer the bash internal loop to prevent unnecessary scope switching between bash within the SUT and perl code on the outside worker environment

That's actually a good point.

I mainly opted for using the helper because it introduces a delay between the attempts (which might otherwise be pointless anyways). However, a delay can also be added on Bash level.

@kalikiana
Copy link
Member Author

Verification run: https://openqa.opensuse.org/tests/2339756

Well, no strong opinion on my side. I figured I'd try what was suggested since it's proven to work well in some cases.

@kalikiana kalikiana closed this May 11, 2022
@okurz okurz deleted the use_script_retry_everywhere branch May 11, 2022 20:11
@grisu48
Copy link

grisu48 commented May 12, 2022

It is based on a suggestion by @grisu48 and yes, I realized after pushing the initial branch that this is a function from distri-opensuse. If you think this is a bad approach, maybe it should be re-considered in os-autoinst-distri-opensuse as well?

Maybe but I leave it to the maintainers of os-autoinst-distri-opensuse to decide that. For this test distribution here I see no benefit

The reason to not use distri-opensuse-specific functions is very valid, however the main advantage of using script_retry in this was was the delay => 60 parameter. The same could have been achieved by adding a sleep to the loop.

This is just for future reference, no need to change anything now.

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