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

Some bugfixes #315

Closed
wants to merge 1 commit into from
Closed

Some bugfixes #315

wants to merge 1 commit into from

Conversation

Schnitzl42
Copy link
Contributor

-Mainly fixes wrong names in docstrings.
-Added admin_pass parameter to the create_node() function of OpenStack_1_1_NodeDriver.
This allows to set the root password that is assigned to a node when creating an instance with Rackspace.
-Added some imports of with_statement to be compatible to Python 2.5.
-Fixed exception when calling list_nodes() with Brightbox.
-Added locations arguments to Brightboxs list_images() to avoid exceptions when providing a location argument.

@Kami
Copy link
Member

Kami commented Jun 11, 2014

Looks good, but can you please sync your branch with trunk and squash all the commits (https://libcloud.readthedocs.org/en/latest/development.html#squash-the-commits-and-generate-the-patch) so it's easier for me to merge the patch (currently I get conflicts when trying to apply the patch)?

@@ -1789,7 +1789,7 @@ def ex_create_image_from_node(self, node, name, block_device_mapping,
params = {'Action': 'CreateImage',
'InstanceId': node.id,
'Name': name,
'Reboot': reboot}
'NoReboot': reboot}
Copy link
Member

Choose a reason for hiding this comment

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

You changed it to use the reversed API argument, this means you should also negate the reboot argument here (not reboot), right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The simplest solution would be to rename "reboot" to "no_reboot",

but I try to avoid changing the signature of ex_create_image_from_node().

To keep "reboot" I could change the docstring to explain the reversed meaning of "reboot".

(setting "reboot" to false causes a reboot...)

To reverse "reboot" using "not reboot" I'd have to change the default value of "reboot" to true.

Gesendet: Mittwoch, 11. Juni 2014 um 10:00 UhrVon: "Tomaz Muraus" notifications@github.comAn: apache/libcloud libcloud@noreply.github.comCc: Schnitzl42 m.devich@gmx.atBetreff: Re: [libcloud] Some bugfixes (#315)

In libcloud/compute/drivers/ec2.py:

@@ -1789,7 +1789,7 @@ def ex_create_image_from_node(self, node, name, block_device_mapping,
params = {'Action': 'CreateImage',
'InstanceId': node.id,
'Name': name,

  •              'Reboot': reboot}
    
  •              'NoReboot': reboot}
    

You changed it to use the reversed API argument, this means you should also negate the reboot argument here (not reboot), right?


Reply to this email directly or view it on GitHub.

Copy link
Member

Choose a reason for hiding this comment

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

I actually think it's better to not expose "implementation details" to the user and call the argument reboot, not not_reboot.

It's more natural for the user / developer if the argument represents a "positive" and not "negative" action.

@Kami
Copy link
Member

Kami commented Jun 12, 2014

@Schnitzl42 Can you please also fix test and lint issues (https://travis-ci.org/apache/libcloud/builds/27384671) and then squash the commits?

@Schnitzl42
Copy link
Contributor Author

Currently Ive got pretty much to do. Im also facing troubles with my local git repository.

Therefore, fixing this issue and uploading a patch can take 2-3 week in the worst case.

I apologize for any inconvenience.

 

Gesendet: Donnerstag, 12. Juni 2014 um 12:47 UhrVon: "Tomaz Muraus" notifications@github.comAn: apache/libcloud libcloud@noreply.github.comCc: Schnitzl42 m.devich@gmx.atBetreff: Re: [libcloud] Some bugfixes (#315)

@Schnitzl42 Can you please also fix test and lint issues (https://travis-ci.org/apache/libcloud/builds/27384671) and then squash the commits?


Reply to this email directly or view it on GitHub.

@Schnitzl42
Copy link
Contributor Author

After fixing issues in my local repo I squashed the commits.

Is there a way to run lint localy?

 

Gesendet: Donnerstag, 12. Juni 2014 um 12:47 UhrVon: "Tomaz Muraus" notifications@github.comAn: apache/libcloud libcloud@noreply.github.comCc: Schnitzl42 m.devich@gmx.atBetreff: Re: [libcloud] Some bugfixes (#315)

@Schnitzl42 Can you please also fix test and lint issues (https://travis-ci.org/apache/libcloud/builds/27384671) and then squash the commits?


Reply to this email directly or view it on GitHub.

@Kami
Copy link
Member

Kami commented Jun 29, 2014

@Schnitzl42 Yes, run tox -e lint.

fixed naming error in docstring

fixed wrong docstring

fixed wrong name in docstring

fixed wrong name in docstring

fixed wrong name in docstring

added adminPass parameter to create node. Allows setting the root password when creating a node

added location to signature, to keep compatibility with super class method

fixed wrong name in docstring

fixed wrong name in docstring

fixed 'NoneType' object is unsubscriptable - exception when calling list_nodes

reordered docstring

added missing colon in docstring

added imports for with statement for python 2.5 compability

style guide test commit

fixed intendation characters

fixed wrong syntax

fixed wrong syntax really
@Schnitzl42
Copy link
Contributor Author

I resolved the errors. How can I commit the patch? Because I didn't create a jira ticket.

 

Gesendet: Sonntag, 29. Juni 2014 um 14:04 UhrVon: "Tomaz Muraus" notifications@github.comAn: apache/libcloud libcloud@noreply.github.comCc: Schnitzl42 m.devich@gmx.atBetreff: Re: [libcloud] Some bugfixes (#315)

@Schnitzl42 Yes, run tox -e lint.


Reply to this email directly or view it on GitHub.

@Kami
Copy link
Member

Kami commented Jun 29, 2014

@Schnitzl42 It's fine, I can generate the patch and merge your changes directly.

@Schnitzl42 Schnitzl42 closed this Jun 29, 2014
asfgit pushed a commit that referenced this pull request Jun 29, 2014
in the Openstack driver.

Closes #315

Signed-off-by: Tomaz Muraus <tomaz@apache.org>
@Kami
Copy link
Member

Kami commented Jun 29, 2014

Merged into trunk. Thanks.

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