Revert "Merge pull request #191 from winniex1/get_property" #200
Conversation
pmasrani
requested review from
KohliDev,
GregBollella,
arunnswamy,
winniex1 and
gkmccready
|
Reply to Glen's comment: After reverting the changes, i tried out load, unload, unload_all, reload and list pkg commands. |
| @@ -40,7 +40,8 @@ | |||
| from Queue import Queue | |||
| from time import sleep | |||
| from abc import ABCMeta, abstractmethod | |||
| from liota.lib.utilities.utility import read_liota_config, sha1sum, validate_named_pipe | |||
|
|
|||
There was a problem hiding this comment.
This is a revert of a commit. I don't want to introduce changes in a revert.
There was a problem hiding this comment.
They are not changes, it is code formatting as this is getting introduced as part of the new commit.
| if command in ["load", "reload", "update"]: | ||
| #----------------------------------------------------------- | ||
| # Use these commands to handle package management tasks | ||
|
|
There was a problem hiding this comment.
Remove the extra line, reformat all the files using autopep format.
There was a problem hiding this comment.
This is a revert of a commit. I don't want to introduce changes in a revert. Ideally, each developer should take care of formatting during initial check-in itself or there should be tests in the code base which fail if there are formatting issues.
There was a problem hiding this comment.
Now the changes are getting introduced in form of code revert and formatting should be done.
There was a problem hiding this comment.
As discussed, formatting and all should not be taken care of as part of a git revert. It should ideally just revert whatever was introduced,
|
|
||
| if ! [ `ps -ef | grep "liotad.py" | grep -v grep | wc -l` -gt 0 ] | ||
| then | ||
| echo "Liota package manager is not running. Please run it first!" | ||
| exit 1 | ||
| exit -1 |
There was a problem hiding this comment.
What is the significance of changing it to "-1"????
I think they should be positive integers as they are independent of the rollback.
There was a problem hiding this comment.
You're right. I missed it in my review. All exit values should be >= 0.
There was a problem hiding this comment.
This is a revert of a commit. I don't want to introduce changes in a revert. This is what happens when we address more than one issue in a single commit/merge :-)
I validated that ICE client is not consuming these values. It is still a bug in liota. Please raise a separate bug for this. Also, exit values 1 and 2 have special meaning, so we should not use them in the return status. Hence, with or without this revert, the bug still needs to be raised.
There was a problem hiding this comment.
This is a new commit which is getting introduced and shouldn't change the fixed issue of exit status under the revert tag. It can be handled in separate commit but the correct code shouldn't be affected due to this commit.
What is the significance of exit values "1" and "2" and "-1" and "-2"?
There was a problem hiding this comment.
Exit code 1 and 2 have special meanings: http://tldp.org/LDP/abs/html/exitcodes.html
As discussed, this is a git revert commit, and we should not introduce new changes.
There was a problem hiding this comment.
That page is describing what bash itself exits with; we can define whatever exit codes we want for when our scripts exit.
| pass | ||
|
|
||
| def validate_named_pipe(self, pipe_file): | ||
| """ |
There was a problem hiding this comment.
I can see comment section added which wasn't part of Winnie`s initial commit. Hence, this is not a revert commit.
There was a problem hiding this comment.
As discussed, this is a revert commit. utility.py and liotapkg.sh where having merge conflicts, and hence, these changes were manually merged.
A revert commit should only take care of reverting the changes, and should not introduce any new changes.
|
The above-mentioned issues should be handled in separate pull request as it changes the existing code in the Liota codebase by the dev reverting the commit. |
|
Reply to Vaibhav: yes agreed. Please raise separate bugs for the issues. Can you please approve the changes if the revert seems fine? |
|
The issue has been opened and should be resolved, approving the pull request; |
This reverts commit 29b7198, reversing
changes made to fb2009d.
Conflicts:
liota/lib/utilities/utility.py
packages/liotad/liotapkg.sh