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 always_rolllback doc and flag icon #1812

Merged
merged 4 commits into from Oct 23, 2018

Conversation

rwx788
Copy link
Member

@rwx788 rwx788 commented Sep 28, 2018

Please, merge only after os-autoinst change.

See poo#30775.

@codecov
Copy link

codecov bot commented Sep 28, 2018

Codecov Report

Merging #1812 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1812      +/-   ##
==========================================
- Coverage    90.3%   90.28%   -0.02%     
==========================================
  Files         140      140              
  Lines       10065    10065              
==========================================
- Hits         9089     9087       -2     
- Misses        976      978       +2
Impacted Files Coverage Δ
lib/OpenQA/Schema/Result/JobModules.pm 87.31% <ø> (ø) ⬆️
lib/OpenQA/Schema.pm 100% <ø> (ø) ⬆️
lib/OpenQA/Utils.pm 91.85% <ø> (ø) ⬆️
lib/OpenQA/WebAPI/Controller/API/V1/Job.pm 88.25% <100%> (ø) ⬆️
lib/OpenQA/Schema/Result/Jobs.pm 92.43% <100%> (ø) ⬆️
lib/OpenQA/Worker/Engines/isotovideo.pm 94.37% <0%> (-1.25%) ⬇️

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 3aaf4f2...dd4ac14. Read the comment docs.

@@ -79,6 +80,7 @@ sub test_flags {
# 'ignore_failure' - if this module fails, it will not affect the overall result at all
# 'milestone' - after this test succeeds, update 'lastgood'
# 'norollback' - don't roll back to 'lastgood' snapshot if this fails
# 'force_reset' - roll back to 'lastgood' snapshot even if test was successful (supported on QEMU backend only)
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we shouldn't name 'QEMU backend' the default - sounds less scary :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, we don't mention it above as well. I will replace with something like "if supported by backend"

Copy link
Contributor

Choose a reason for hiding this comment

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

It didn't bother me on the first review - but I just had the thought that force_reset sounds a lot like a boot of the SUT. In the light of 'norollback', how about always_rollback (and possibly renaming norollback to no_rollback).

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 also thought about different name, but grabbed one from the progress ticket. As we use norollback just in a single place, it's easy change. So +1 here.

@@ -40,8 +40,9 @@ openQA tests need to implement at least the *run* subroutine to
contain the actual test code and the test needs to be loaded in the distribution's
main.pm.

The *test_flags* subroutine specifies what happens when the test
fails.
The *test_flags* subroutine specifies what should happen when test execution is finished.
Copy link
Member

Choose a reason for hiding this comment

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

In before it stated "if test fails" which would refer to each failure within a job. Now it states "when test execution is finished" which I consider ambiguous. How about "when the execution of the current test module finishes depending on the result …"

@rwx788 rwx788 changed the title Add force_reset flag description [WIP] Add force_reset flag description Oct 1, 2018
@rwx788 rwx788 changed the title [WIP] Add force_reset flag description [WIP] Add always_rolllback doc and flag icon Oct 1, 2018
@rwx788
Copy link
Member Author

rwx788 commented Oct 1, 2018

Test for the change is in progress. Looking for a suiting place for it.

@rwx788 rwx788 force-pushed the automated_comment branch 2 times, most recently from 4f4631e to 279ffb1 Compare October 2, 2018 09:52
@rwx788 rwx788 changed the title [WIP] Add always_rolllback doc and flag icon Add always_rolllback doc and flag icon Oct 2, 2018
@rwx788 rwx788 force-pushed the automated_comment branch 7 times, most recently from af6dc3d to 4969be6 Compare October 16, 2018 10:39

;
ALTER TABLE job_modules ADD COLUMN always_rollback integer DEFAULT 0 NOT NULL;
Copy link
Contributor

Choose a reason for hiding this comment

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

this doesn't work - you need a new schema version

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately, I was able only to find the documentation to introduce changes to the DB this way. Do you think @Martchus can give me a hand here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I can, but I usually just go through these steps myself: https://github.com/os-autoinst/openQA/blob/master/docs/Contributing.asciidoc#managing-the-database

You forgot step 1. here (updating the schema version number) and instead overwrote the previous migration files.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I see. Will fix that. Thanks a lot!

@rwx788 rwx788 force-pushed the automated_comment branch 5 times, most recently from 2465bab to c2cf168 Compare October 17, 2018 13:09
@coolo
Copy link
Contributor

coolo commented Oct 17, 2018

I'm still missing a commit in Schema.pm

@rwx788
Copy link
Member Author

rwx788 commented Oct 17, 2018

@coolo sorry, messed up this PR with reverts, so now it can be looked at.

Copy link
Contributor

@Martchus Martchus left a comment

Choose a reason for hiding this comment

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

Looks good now.

It conflicts with #1835 so the PR which is merged last needs to rebase.

@coolo
Copy link
Contributor

coolo commented Oct 17, 2018

@Martchus your call then in which order. 1835 is more urgent, but I expect you to be more fluent in rebaing

@rwx788
Copy link
Member Author

rwx788 commented Oct 17, 2018

@coolo as this PR is not urgent, I can also take my time for rebasing, so no issue with that.

@Martchus
Copy link
Contributor

Rebasing should be quite easy in this case so it doesn't really matter - I've just noted it.

@rwx788
Copy link
Member Author

rwx788 commented Oct 17, 2018

Conflict resolved. Should be fine now.

@coolo coolo merged commit 587a775 into os-autoinst:master Oct 23, 2018
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