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

Fix misleading error message on non-UEFI jobs #1825

Merged
merged 1 commit into from Oct 15, 2018

Conversation

okurz
Copy link
Member

@okurz okurz commented Oct 11, 2018

@okurz
Copy link
Member Author

okurz commented Oct 11, 2018

@richiejp do you think this is the right approach?

@codecov
Copy link

codecov bot commented Oct 11, 2018

Codecov Report

Merging #1825 into master will increase coverage by 0.1%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #1825     +/-   ##
=========================================
+ Coverage   90.18%   90.28%   +0.1%     
=========================================
  Files         139      139             
  Lines        9982     9983      +1     
=========================================
+ Hits         9002     9013     +11     
+ Misses        980      970     -10
Impacted Files Coverage Δ
lib/OpenQA/Worker/Engines/isotovideo.pm 95.62% <100%> (+0.02%) ⬆️
lib/OpenQA/WebSockets/Server.pm 92.2% <0%> (+1.83%) ⬆️
lib/OpenQA/Scheduler/Scheduler.pm 88.58% <0%> (+2.28%) ⬆️
lib/OpenQA.pm 71.79% <0%> (+2.56%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3d1e3f3...876afa2. Read the comment docs.

@okurz
Copy link
Member Author

okurz commented Oct 11, 2018

hm, could it be there is a big coverage drop reported because I did not rebase my openQA working copy?

@okurz
Copy link
Member Author

okurz commented Oct 11, 2018

Seems like it

@richiejp
Copy link
Contributor

Well, it is either this or change it to a warning instead of an error if it is UEFI flash that is missing.

@okurz
Copy link
Member Author

okurz commented Oct 12, 2018

hm, I don't understand how the warning would help or what it actually warns about when the asset is not expected on non-UEFI jobs.

@ggardet
Copy link
Contributor

ggardet commented Oct 12, 2018

Why do you check the UEFI var set to 0? In my case (aarch64) UEFI is set to 1.
See: https://openqa.opensuse.org/tests/771579#settings

@okurz
Copy link
Member Author

okurz commented Oct 12, 2018

Yes, because the presence of the asset is only required on UEFI, not otherwise. Mentioning this PR to you in the ticket was a mistake by me because aarch64 is always UEFI in this case.

@coolo coolo merged commit 99583ca into os-autoinst:master Oct 15, 2018
@okurz okurz deleted the fix/uefi_pflash_error branch October 15, 2018 20:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants