-
Notifications
You must be signed in to change notification settings - Fork 23
Merge SSH management feature to XS8 #67
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
Conversation
Currently, xsconsole uses systemd to manage ssh service. In the ssh configuration feature (CP-50702), ssh's status is saved in xapi DB and is updated by xapi API. So change the way of managing ssh to xapi API in xsconsole to keep the ssh's status updating. Signed-off-by: Bengang Yuan <[email protected]>
Signed-off-by: Lunfan Zhang <[email protected]>
Signed-off-by: Lunfan Zhang <[email protected]>
Signed-off-by: Lunfan Zhang[Lunfan.Zhang] <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi,
I'd like to propose to please rename the global variables DISABLE, ENABLE and AUTO to be more specific (SSH_MODE_ENABLE, SSH_MODE_ENABLE, SSH_MODE_AUTO.
To not use the global scope unless actually were needed (global variables do not have a great reputation and are lumped to gether on the top of the file), I'd like to propose to move them to class variables of the class that uses them and then let those classes referto them using self.SSH_MODE_ENABLE, self.SSH_MODE_ENABLE, self.SSH_MODE_AUTO.
This makes the calls that use these variables as arguments more self-explanatory. Comments would be an alternative, but not a great alternative, as adding comments is not needed when the variable names are self-explanatory.
Hi, I have refactor the variable name #68, and I can not move these variable to class, as there are two class |
Signed-off-by: Lunfan Zhang[Lunfan.Zhang] <[email protected]>
Signed-off-by: Lunfan Zhang[Lunfan.Zhang] <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, updates done! I set my comments to resolved! Approving hereby!
As title, merge SSH management feature to XS8