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

Reserve public IP block #46724

Merged
merged 9 commits into from Apr 4, 2018
Merged

Conversation

edevenport
Copy link
Contributor

What does this PR do?

Provides a CLI function allowing IP blocks to be reserved. The public IPs can then be used in the cloud profile.

Previous Behavior

IP blocks had to be reserved via the provider UI or other tools.

New Behavior

IP blocks can be reserved from the salt-cloud CLI.

Tests written?

No

Commits signed with GPG?

No

@@ -151,6 +151,14 @@ command:

# salt-cloud --list-sizes my-profitbricks-config

.. versionadded:: Fluorine
One or more public IP address can be reserved with the following command:
command:
Copy link
Contributor

Choose a reason for hiding this comment

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

duplicate command?

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed

@@ -120,7 +120,7 @@
import profitbricks
from profitbricks.client import (
ProfitBricksService, Server,
NIC, Volume, FirewallRule,
NIC, Volume, FirewallRule, IPBlock,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is IPBlock a new addition to this upstream library or has it been present for a while? We want to make sure we preserve backward compatibility.

@@ -624,6 +626,40 @@ def list_nodes_full(conn=None, call=None):
return ret


def reserve_ipblock(call=None, kwargs=None):
'''

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add docs here, please?

Copy link
Contributor

Choose a reason for hiding this comment

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

Doc comment added

@@ -120,7 +120,7 @@
import profitbricks
from profitbricks.client import (
ProfitBricksService, Server,
NIC, Volume, FirewallRule,
NIC, Volume, FirewallRule, IPBlock,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is IPBlock new to the latest release of profitbricks? The minimum version required is 3.1.0 according to the docs in this file. This import needs to be handled more elegantly if someone is using profitbricks with an older version.

@denza
Copy link
Contributor

denza commented Mar 28, 2018

@rallytime @cachedout
IP Block feature is present in the Profitbricks SDK/API from the first version. This is just the first time we are making this feature available in the salt cli. This new method is just one part of a larger project where we are trying to improvement usability of the network management.

@denza
Copy link
Contributor

denza commented Apr 2, 2018

@rallytime @cachedout Could you please let us know if you require any further changes.

Copy link
Contributor

@rallytime rallytime left a comment

Choose a reason for hiding this comment

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

Hi @edevenport and @denza I just have one small request on this, otherwise this LGTM.

size = kwargs.get('size')

block = conn.reserve_ipblock(IPBlock(size=size, location=location))
pprint.pprint(block)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you remove this print statement? You can use logging if necessary, but we don't like to print directly this way.

Copy link
Contributor

Choose a reason for hiding this comment

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

@rallytime The print statement removed.

@rallytime rallytime merged commit 92328dc into saltstack:develop Apr 4, 2018
@edevenport edevenport deleted the reserve_public_ip branch April 10, 2018 05:38
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.

None yet

4 participants