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.

Sign up for GitHub

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

Jump to bottom

Fail CI check if 'record_soft_failure' without valid reference #15615

Merged
merged 1 commit into from
Oct 10, 2022
Merged

Fail CI check if 'record_soft_failure' without valid reference #15615

merged 1 commit into from
Oct 10, 2022

Conversation

rfan1
Copy link
Contributor

@rfan1 rfan1 commented Sep 29, 2022 •
edited
Loading

@github-actions
Copy link

Great PR! Please pay attention to the following items before merging:

Files matching lib/**.pm:

  • Consider adding or extending unit tests in t/

This is an automatically generated QA checklist based on modified files

@rfan1 rfan1 force-pushed the soft_fail_to_record_info branch 5 times, most recently from aa65ca4 to 7e7f589 Compare September 29, 2022 06:22
@rfan1 rfan1 changed the title [wip]Fail CI check if 'record_soft_failure' without valid reference Fail CI check if 'record_soft_failure' without valid reference Sep 29, 2022
okurz
okurz reviewed Sep 29, 2022
View reviewed changes
@rfan1 rfan1 force-pushed the soft_fail_to_record_info branch from 7e7f589 to 5a02a16 Compare September 30, 2022 01:27
@foursixnine foursixnine merged commit e6dba7d into os-autoinst:master Oct 10, 2022
@mpagot mpagot self-requested a review October 10, 2022 09:48
mpagot
mpagot reviewed Oct 10, 2022
View reviewed changes
lib/trento.pm
@@ -602,7 +602,7 @@ sub cypress_exec {
$self->result("fail");
}
if ($failok) {
record_soft_failure("Cypress exit code:$ret at $log_prefix") if ($ret);
record_info('Softfail', "Cypress exit code:$ret at $log_prefix", result => 'softfail') if ($ret);
Copy link
Contributor

Choose a reason for hiding this comment

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

ok

alvarocarvajald
alvarocarvajald reviewed Oct 10, 2022
View reviewed changes
tests/sles4sap/sys_param_check.pm
@@ -36,7 +36,7 @@ sub add_softfail {
my ($module, $os_version, $bsc_number, @parameters) = @_;
foreach my $parameter (@parameters) {
if (check_var("VERSION", $os_version) && check_failure($module, $parameter)) {
record_soft_failure "$bsc_number - Wrong value for $parameter";
record_info('Softfail', "$bsc_number - Wrong value for $parameter", result => 'softfail');
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to use record_soft_failure here to mark the whole job as soft failed and record_info will not do that. As can be seen, the message to record_soft_failure does include a bsc# (via a variable). Is there a way to include this again?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm, I can add exception for this dedicated string "bsc_number"

foursixnine added a commit to foursixnine/os-autoinst-distri-opensuse that referenced this pull request Oct 24, 2023
@foursixnine
Fix rogue record_info introduced by os-autoinst#15615
30cf8cb 
In this particular case, we missed a variable that was should have been
named reference in the first place should help with os-autoinst#18062
foursixnine added a commit to foursixnine/os-autoinst-distri-opensuse that referenced this pull request Oct 24, 2023
@foursixnine
Fix rogue record_info introduced by os-autoinst#15615
4100225 
In this particular case, we missed a variable that was should have been
named reference in the first place should help with os-autoinst#18062
foursixnine added a commit to foursixnine/os-autoinst-distri-opensuse that referenced this pull request Oct 24, 2023
@foursixnine
Fix rogue record_info introduced by os-autoinst#15615
64084a8 
In this particular case, we missed a variable that was should have been
named reference in the first place should help with os-autoinst#18062
foursixnine added a commit to foursixnine/os-autoinst-distri-opensuse that referenced this pull request Oct 24, 2023
@foursixnine
Fix rogue record_info introduced by os-autoinst#15615
cc5248f 
In this particular case, we missed a variable that was should have been
named reference in the first place should help with os-autoinst#18062
foursixnine added a commit to foursixnine/os-autoinst-distri-opensuse that referenced this pull request Oct 24, 2023
@foursixnine
Fix rogue record_info introduced by os-autoinst#15615
04dc22a 
In this particular case, we missed a variable that was should have been
named reference in the first place should help with os-autoinst#18062
foursixnine added a commit that referenced this pull request Oct 24, 2023
@foursixnine
Merge pull request #18065 from foursixnine/oopsitsbrokenagain
5e07608 
Fix rogue record_info introduced by #15615
@foursixnine foursixnine mentioned this pull request Oct 24, 2023
Draft
ana added a commit to ana/os-autoinst-distri-opensuse that referenced this pull request Oct 26, 2023
@ana
Revert "Fix rogue record_info introduced by os-autoinst#15615"
ead3855 
This reverts commit 04dc22a.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@foursixnine foursixnine foursixnine approved these changes

@okurz okurz okurz left review comments

@alvarocarvajald alvarocarvajald alvarocarvajald left review comments

@mpagot mpagot mpagot left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@rfan1 @foursixnine @okurz @alvarocarvajald @mpagot