projectatomic / atomicapp

[UNMAINTAINED] This is the reference implementation of the Nulecule container application Specification: Atomic App

102 stars 71 forks source link

handle_exception #770

Closed procrypt closed 8 years ago

procrypt commented 8 years ago

This pr will handle the exception in issue #620 @cdrage @dustymabe Please review it.

centos-ci commented 8 years ago

Can one of the admins verify this patch?

centos-ci commented 8 years ago

Can one of the admins verify this patch?

centos-ci commented 8 years ago

Can one of the admins verify this patch?

centos-ci commented 8 years ago

Can one of the admins verify this patch?

procrypt commented 8 years ago

@cdrage is this what you were saying ?

dustymabe commented 8 years ago

@abhi1004 I don't understand this PR. How does it relate to #620?

procrypt commented 8 years ago

@dustymabe as you mentioned in the comment here https://github.com/projectatomic/atomicapp/pull/726#issuecomment-220607118 that subprocess.check_output() raise an exception when the command fails so I thought we should handle that exception. My bad I didn't mentioned it before in the comment.

coveralls commented 8 years ago

Coverage decreased (-0.3%) to 56.676% when pulling c596d452d85071c98d67f1a962f800005ae07871 on abhi1004:handle_exception into d85321322d5d747d7feda8d9edab8d1eefaf810f on projectatomic:master.

procrypt commented 8 years ago

@cdrage I followed pattern as here https://github.com/projectatomic/atomicapp/pull/753/files and made the changes. All tests pass locally. @dustymabe Yes, not related to #620, but related to the https://github.com/projectatomic/atomicapp/pull/726#issuecomment-220607118 Let me know if you need me to do anything else with the issue.

cdrage commented 8 years ago

dotests

cdrage commented 8 years ago

Commented on removing Failed! from the logging output. But other than that this LGTM. @dustymabe ?

dustymabe commented 8 years ago

@cdrage LGTM - only thing I would change is to cut lines down to below 80 characters

procrypt commented 8 years ago

@cdrage @dustymabe made the changes as asked.

cdrage commented 8 years ago

dotests

coveralls commented 8 years ago

Coverage decreased (-0.3%) to 56.676% when pulling a47af88f7b5fc6162be775f334c0dfebae7e3e82 on abhi1004:handle_exception into d85321322d5d747d7feda8d9edab8d1eefaf810f on projectatomic:master.

dustymabe commented 8 years ago

LGTM - but all tests failed? @cdrage can you investigate and then merge when ready.

procrypt commented 8 years ago

@dustymabe all tests passed locally, I checked it before sending the PR.

cdrage commented 8 years ago

@dustymabe openshift + k8s tests are false positive due to nulecule-library examples updating. docker one timed out. will run tests again once nulecule-library etherpad example has been updated

cdrage commented 8 years ago

dotests

cdrage commented 8 years ago

Ayyyy tests pass! :) LGTM Merge when ready.