-
Notifications
You must be signed in to change notification settings - Fork 266
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
Extension for qam-minimal tests (poo#17412) #2563
Conversation
ab85ca9
to
c55b9ed
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added some comments. Apart from that, LGTM from me ;)
my $patch_info = shift; | ||
my $pattern = qr/\s+(.+)(?!\.src)\..*\s<\s.*/; | ||
|
||
# loop over packages in patchinfo and try instalation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: installation
sub run { | ||
my ($self) = @_; | ||
my $patch = get_var('INCIDENT_PATCH'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
before declaring this to the patch
variable, it would be a good practice to make sure that the env variable INCIDENT_PATCH
exists and it's not empty. e.g.
# Check if INCIDENT_PATCH variable exists and has a non-empty value
die "openQA Error -> Cannot find the maintenance incident patch" unless get_var('INCIDENT_PATCH');
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this, isnt needed , it always defined by test settings. In reality we can remove INCINDENT_REPO test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If they are not needed, then for the sake of consistency I would suggest to treat all the settings
in the same way. So, either remove the check for both, or add it for both ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, keep the case in mind that the variable is undefined. get_var
can also take an optional default value, e.g. get_var('PATCH', 'foo')
. If you rely on the variable being defined, use get_required_var()
.
c55b9ed
to
e03c24e
Compare
If test is qam-minimal mark test softfailed wheen patch isn't needed in minimal pattern. And skip patch installation. For qam-minimal-full test records info, if patch isn't needed for minimal pattern. If patch isn't applied in install_patterns step installs packages mentioned in patchinfo.
e03c24e
to
e5dca02
Compare
Ping? |
If test is qam-minimal mark test softfailed wheen patch isn't needed in
minimal pattern. And skip patch installation.
For qam-minimal-full test records info, if patch isn't needed for
minimal pattern. If patch isn't applied in install_patterns step installs
packages mentioned in patchinfo.
http://argus.suse.cz/tests/186
http://argus.suse.cz/tests/185
http://argus.suse.cz/tests/187