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

[LIBCLOUD-526] Add CloudStackProject class and tests, and ability to create node with project. #257

Closed
wants to merge 5 commits into from

Conversation

@jdivine
Copy link
Contributor

jdivine commented Mar 6, 2014

jdivine added 2 commits Mar 5, 2014
…ect and disk offering on node creation. Add tests.
@Kami
Copy link
Member

Kami commented Mar 7, 2014

@jdivine Thanks, I will do a code review later today.

/cc @Runseb

Class representing a CloudStack Project.
"""

def __init__(self, displaytext, name, id,

This comment has been minimized.

@Kami

Kami Mar 7, 2014 Member

For consistency, displaytext should be display_text.

This comment has been minimized.

@Kami

Kami Mar 7, 2014 Member

I also think, arguments should be defined in a different order. So:

def __init__(self, id, name display_text, driver, extra=None)
if 'tags' in proj:
extra['tags'] = self._get_resource_tags(proj['tags'])

projects.append(CloudStackProject(

This comment has been minimized.

@Kami

Kami Mar 7, 2014 Member

Minor style thing - please prefer keyword over regular arguments. So:

CloudStackProject(id=proj['id'], ...)
@Kami
Copy link
Member

Kami commented Mar 7, 2014

Lint step appears to be failing - https://travis-ci.org/apache/libcloud/jobs/20244959

@Kami
Copy link
Member

Kami commented Mar 7, 2014

I've added some comments. Besides that, PR looks good to me.

@sebgoa
Copy link
Member

sebgoa commented Mar 7, 2014

there is some new indentation in the list_volumes and ex_list_port_forwarding methods that are most likely breaking pep8.

…ect and disk offering on node creation. Add tests.
@jdivine
Copy link
Contributor Author

jdivine commented Mar 7, 2014

OK, I've made the suggested changes. My IDE changed some indentation which caused flake8 to fail. "displaytext" is used on other types in this driver (e.g. CloudStackNetwork) but I have made the change to "display_text" as suggested. Thank you.

jdivine added 2 commits Mar 7, 2014
…ect and disk offering on node creation. Add tests.
…ect and disk offering on node creation. Add tests.
@jdivine
Copy link
Contributor Author

jdivine commented Mar 7, 2014

Fixed a couple of other indentation issues that cropped up under the docs phase.

@Kami
Copy link
Member

Kami commented Mar 7, 2014

Merged into trunk.

Thanks.

@asfgit asfgit closed this in 583bfba Mar 7, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.