Skip to content
This repository has been archived by the owner on Nov 9, 2020. It is now read-only.

Fixed error on parsing PID for status command. #641

Merged
merged 1 commit into from
Oct 26, 2016
Merged

Fixed error on parsing PID for status command. #641

merged 1 commit into from
Oct 26, 2016

Conversation

govint
Copy link
Contributor

@govint govint commented Oct 24, 2016

Also added a unit test for status command.

@govint
Copy link
Contributor Author

govint commented Oct 24, 2016

Fixes #633

Copy link
Contributor

@msterin msterin left a comment

Choose a reason for hiding this comment

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

CI fails in what seems to be related areas, will take a look when CI passes.
meanwhile - can you elaborate on what was the error (output before/after the fix) and what config is needed to repro ?

@@ -591,7 +591,8 @@ def get_service_status():
if output[2] == "not":
return NOT_RUNNING_STATUS

pid = output[3].split("=")[1]
Copy link
Contributor

Choose a reason for hiding this comment

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

please add a comment with expected result example (makes easy to understand and check the code).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was the error:

File "/usr/lib/vmware/vmdkops/bin/vmdkops_admin.py", line 594, in get_service_status
pid = output[3].split("=")[1]
TypeError: a bytes-like object is required, not 'str'

Copy link
Contributor

Choose a reason for hiding this comment

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

that's OK - but can you please add a comment (in the code) with the example of output so the output[3].split("=")[1] becomes clear ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, and this is the fixed output:

python /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py status
Version: 0.7.3f2c66a-0.0.1
Status: Running
Pid: 5585696
Port: 1019
LogConfigFile: /etc/vmware/vmdkops/log_config.json
LogFile: /var/log/vmware/vmdk_ops.log

@msterin
Copy link
Contributor

msterin commented Oct 25, 2016

the fix looks good and good to go when CI passes

@pdhamdhere
Copy link
Contributor

You may want to check CI, vmdkopsAdmin testStatus test is failing.

@govint
Copy link
Contributor Author

govint commented Oct 25, 2016

Its a different test thats failing (vmdkops_admin_sanity_test.py), the new
test was added to vmdkops_admin_test.py

On Tue, Oct 25, 2016 at 9:39 AM, Govindan T tgovin1@gmail.com wrote:

Restarted CI test for this change I'll check and update.

On Tue, Oct 25, 2016 at 9:34 AM, Prashant Dhamdhere <
notifications@github.com> wrote:

You may want to check CI, vmdkopsAdmin testStatus test is failing.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#641 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/APHseMor7tefqvnaK96fKbK-JevIC19aks5q3X-5gaJpZM4Kerhw
.

@govint
Copy link
Contributor Author

govint commented Oct 25, 2016

More byte to string handling in test code. Fixed.

On Tue, Oct 25, 2016 at 10:05 AM, Govindan T tgovin1@gmail.com wrote:

Its a different test thats failing (vmdkops_admin_sanity_test.py), the
new test was added to vmdkops_admin_test.py

On Tue, Oct 25, 2016 at 9:39 AM, Govindan T tgovin1@gmail.com wrote:

Restarted CI test for this change I'll check and update.

On Tue, Oct 25, 2016 at 9:34 AM, Prashant Dhamdhere <
notifications@github.com> wrote:

You may want to check CI, vmdkopsAdmin testStatus test is failing.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#641 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/APHseMor7tefqvnaK96fKbK-JevIC19aks5q3X-5gaJpZM4Kerhw
.

@govint
Copy link
Contributor Author

govint commented Oct 25, 2016

Restarted CI test for this change I'll check and update.

On Tue, Oct 25, 2016 at 9:34 AM, Prashant Dhamdhere <
notifications@github.com> wrote:

You may want to check CI, vmdkopsAdmin testStatus test is failing.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#641 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/APHseMor7tefqvnaK96fKbK-JevIC19aks5q3X-5gaJpZM4Kerhw
.

Copy link
Contributor

@pdhamdhere pdhamdhere left a comment

Choose a reason for hiding this comment

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

LGTM

@pdhamdhere pdhamdhere merged commit a6b735f into master Oct 26, 2016
@msterin msterin deleted the cli branch October 28, 2016 23:59
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants