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

Add third party plugins #26

Open
wants to merge 6 commits into
base: chameleoncloud/xena
Choose a base branch
from

Conversation

Mark-Powers
Copy link

No description provided.

@Mark-Powers Mark-Powers marked this pull request as ready for review June 12, 2023 19:55
@Mark-Powers Mark-Powers requested a review from super-cooper June 12, 2023 19:55
Copy link

@super-cooper super-cooper left a comment

Choose a reason for hiding this comment

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

Code is good, and fits in nicely with the rest of blazarclient! There are a few very small things which I think could use come cleaning up.

Comment on lines 98 to 107
if hasattr(parsed_args, "resource") and hasattr(blazar_client, parsed_args.resource):
resource_manager = getattr(blazar_client, parsed_args.resource)
if add_to_body:
args["resource_type"] = self.resource
elif hasattr(parsed_args, "resource"): # If third party resource
resource_manager = blazar_client.resource
if add_to_body:
args["resource_type"] = parsed_args.resource
else:
args.insert(0, parsed_args.resource)

Choose a reason for hiding this comment

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

This branching is a little difficult to parse at first glance. I prefer consolidating a little bit

if hasattr(parsed_args, "resource"):
    if hasattr(blazar_client, parsed_args.resource)
        resource_manager = getattr(blazar_client, parsed_args.resource)
        resource_type = self.resource
    else:
        resource_manager = blazar_client.resource
        resource_type = parsed_args.resource
    if add_to_body:
        args["resource_type"] = resource_type

resource_manager = getattr(blazar_client, self.resource)
resource_manager.update(res_id, **body)
resource_manager, args = self.get_manager_and_args(parsed_args, [res_id], add_to_body=False, blazar_client=blazar_client)
data = resource_manager.update(*args, **body)

Choose a reason for hiding this comment

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

This variable appears unused

@@ -206,16 +228,15 @@ def get_parser(self, prog_name):
def run(self, parsed_args):
self.log.debug('run(%s)' % parsed_args)
blazar_client = self.get_client()
resource_manager = getattr(blazar_client, self.resource)
res_id = parsed_args.id

Choose a reason for hiding this comment

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

This gets overwritten immediately if self.allow_names is true. It should probably be in an else branch to be more clear.

def list_resources(self, sort_by=None):
resp, body = self.request_manager.get('/resources')
if sort_by:
resources = sorted(body, key=lambda l: l[sort_by])

Choose a reason for hiding this comment

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

This variable is unused, and sorted produces a copy, so nothing will actually be sorted to the user.

Copy link
Author

Choose a reason for hiding this comment

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

fixed, should be body =

import logging
LOG = logging.getLogger(__name__)

class ResourceClientManager(base.BaseClientManager):

Choose a reason for hiding this comment

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

Most of the dict lookups in this class assume that the server will also have plugins installed. Is this assumption safe to make?

Copy link
Author

Choose a reason for hiding this comment

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

I think this is fine, as blazar already assumes you have oshosts/network/fip running. There is a /resources endpoint in 3rd party plugins patch which shows enabled resources we could check, but that would require an extra API call for each command.

return body['resource']

def update(self, resource_type, res_id, data, extras):
LOG.info("RESOURCE CLIENT UPDATE")

Choose a reason for hiding this comment

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

Is this leftover debugging?

Copy link
Author

Choose a reason for hiding this comment

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

oops

return body['resource']

def delete(self, resource_type, resource_id):
resp, body = self.request_manager.delete(

Choose a reason for hiding this comment

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

These variables are unused

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