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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue #110 - Made GUI port optionally configurable #120

Merged
merged 5 commits into from
Aug 23, 2019

Conversation

robschneider16
Copy link

Instead of defaulting to port 8080, the ait gui should now look at the
ait config and pull the defined gui.port number from there if its defined.
If it is not defined, then it will use the default of 8080. All of which
gets overwritten if the user provided a port number to the init method

Instead of defaulting to port 8080, the ait gui should now look at the
ait config and pull the defined gui.port number from there if its defined.
If it is not defined, then it will use the default of 8080. All of which
gets overwritten if the user provided a port number to the init method
@robschneider16 robschneider16 requested review from a team as code owners August 6, 2019 18:42
@MJJoyce
Copy link
Member

MJJoyce commented Aug 7, 2019

Hey @robschneider16, thanks for the pull request. The changes here definitely cover the functionality we need but there's a bit of a change in our config layout now that we've switched over to the new server architecture. The init function in the GUI is leftover from the "old API" and Plugins can be passed additional configuration arguments beyond the required attributes. The gui.port attribute (really all the old GUI config stuff) isn't going to stick around in the future since we expect all necessary config for a Plugin to be defined under that Plugin's initialization block.

plugins:
    - plugin:
        name: ait.server.plugins.ait_gui_plugin.AITGUIPlugin
        inputs:
            - log_stream
            - telem_stream
        outputs:
            - command_stream
        other_parameter: anything_you_need

Here, other_parameter is not a required parameter and will be accessible in the Plugin's __init__ function via kwargs. I would suggest updating the Plugins __init__ to pull a parameter called port from kwargs and pass that to the gevent.spawn(self.init) call. That will get us the functionality we need and when we clean up this Plugin and (likely) drop init we'll have to change less.

Thoughts?

Robert Schneider added 2 commits August 13, 2019 12:36
Action requested from the PR review. Moved the port argument to the
plugin class init as a kwarg with a default to port 8080. Kwarg gets
populated based on whats defined in config.yaml.
Instead of passing in the values to the GUIs self.init method,
the init method will see if the plugin has the port and host items
defined as attributes.
@@ -255,7 +255,14 @@ def handle(pathname):
def handle(pathname):
return bottle.static_file(pathname, root=HTMLRoot.User)

if host is None:
if hasattr(self, 'port'):
Copy link
Member

Choose a reason for hiding this comment

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

I realize that it's not a problem in this instance but we should avoid hasattr in 2.x in my opinion. Of course, we're moving to 3.x shortly and it won't matter at all but sticking with try ... except here in 2.x is safer in the general case (it's also the same number of lines in this example). I also realize this is already present in the code so I don't blame you much =D. Don't need to change it, just a general comment for future consideration.

Soap boxing aside, isn't getattr cleaner here or is there a reason for the expanded check?

port = int(getattr(self, 'port', 8080))

if host is None:
host = 'localhost'
port = int(getattr(self, 'port', 8080)):
host = getattr(self, 'port', 'localhost'):
Copy link
Member

Choose a reason for hiding this comment

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

I think a typo snuck in here @robschneider16

host = getattr(self, 'port', 'localhost'):

should be

host = getattr(self, 'host', 'localhost'):

@MJJoyce MJJoyce merged commit e46f882 into NASA-AMMOS:master Aug 23, 2019
@MJJoyce
Copy link
Member

MJJoyce commented Aug 23, 2019

Thanks @robschneider16!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants