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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Skip ZFCP warning on zKVM + code cleanup #44

Merged
merged 76 commits into from Apr 24, 2017
Merged

Skip ZFCP warning on zKVM + code cleanup #44

merged 76 commits into from Apr 24, 2017

Conversation

@lslezak
Copy link
Member

lslezak commented Apr 24, 2017

This fixes bug bsc#1014178 - do not display a warning when a ZFCP controller is not detected in zKVM (zKVM uses virtio devices, the warning is pointless and confusing).

The fix itself is just a trivial Arch.is_zkvm test, however it turned out that the package was in a quite bad shape and needed some cleanup.

Additional Cleanup

  • Improved tests
    • Use shared test_helper.rb
    • Added code coverage support (Coveralls) + a badge in README.md
    • Moved big inline data structures into separate YAML files for better readability
    • Dropped the old testsuite (was useless anyway)
    • Mock sleep() to make a test a bit faster
  • Added Rubocop support
  • Cleanup
    • Removed empty agents/ subdir
    • Updated .gitignore
  • Updated Dockerfile so Travis builds properly

Review Notes

The changes with Rubocop fix: commit comment were done automatically by Rubocop in auto-correct mode. You can do just a quick scan or even skip them if you trust Rubocop as I do. (On the other hand the Rubocop manual fix: changes were done manually so these should be checked carefully... 馃槈)

lslezak added 30 commits Apr 20, 2017
ZKVM uses virtio devices instead of ZFCP controllers,
the warning is useless and confusing.
lslezak added 18 commits Apr 21, 2017
Mock the sleep() call.
@coveralls

This comment has been minimized.

Copy link

coveralls commented Apr 24, 2017

Coverage Status

Changes Unknown when pulling 9a017f8 on zkvm into ** on master**.

@@ -2,4 +2,5 @@ FROM yastdevel/ruby
COPY . /usr/src/app
# a workaround to allow package building on a non-s390 machine
RUN sed -i "/^ExclusiveArch:/d" package/*.spec

# add the change so "check:committed" task does not fail

This comment has been minimized.

Copy link
@jreidinger

This comment has been minimized.

Copy link
@lslezak

lslezak Apr 24, 2017

Author Member

We want to build the package in Travis, but Travis runs in an x86_86 VM. And because of the ExclusiveArch tag in the spec file the build normally fails.

There is hack on the previous line which removes the tag to allow package build. But this change is found by the rake package call (by the check:committed subtask) as an uncommited change and the build fails.

So this workaround adds the change to index so rake does not fail. Not nice but I'm not sure if there is a better solution.

@@ -367,7 +368,8 @@ def GetControllers
Builtins.contains(["sysfs_bus_id"], k)
end }

if ret_vmcp != 0 && @controllers.size == 0
# zKVM uses virtio devices instead of ZFCP, skip the warning in that case
if ret_vmcp != 0 && @controllers.size == 0 && !Arch.is_zkvm

This comment has been minimized.

Copy link
@jreidinger

jreidinger Apr 24, 2017

Member

@controllers.size == 0 maybe @controllers.empt? ?

@coveralls

This comment has been minimized.

Copy link

coveralls commented Apr 24, 2017

Coverage Status

Changes Unknown when pulling 21a76c6 on zkvm into ** on master**.

Copy link
Member

jreidinger left a comment

LGTM

Changes
- 3.2.1
@coveralls

This comment has been minimized.

Copy link

coveralls commented Apr 24, 2017

Coverage Status

Changes Unknown when pulling ffbd454 on zkvm into ** on master**.

@lslezak lslezak merged commit 48b3020 into master Apr 24, 2017
3 checks passed
3 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls First build on master at 3.861%
Details
@lslezak lslezak deleted the zkvm branch Apr 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can鈥檛 perform that action at this time.