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

ProfitBricks Cloud API v4 support #44987

Merged
merged 12 commits into from Jan 9, 2018

Conversation

@edevenport
Copy link
Contributor

@edevenport edevenport commented Dec 14, 2017

What does this PR do?

Updates to support the ProfitBricks Cloud API v4:

  • Support for the image aliases.
  • Adding new location.

Expanding ProfitBricks cloud provider for a salt-cloud tool to support more commands:

  • list-images
  • list-locations
  • create_datacenter
  • Extended configuration with a delete_volume option.

Bug fixes:

  • Server vmstate vs state response.

Documentation and test updates.

Tests written?

Yes

Commits signed with GPG?

No

@rallytime rallytime requested a review from gtmanfred Dec 15, 2017
@rallytime
Copy link
Contributor

@rallytime rallytime commented Dec 15, 2017

@tonybaloney Can you review this when you get a moment?

@gtmanfred
Copy link
Contributor

@gtmanfred gtmanfred commented Dec 15, 2017

What are the reasons we are forcing an upgrade to api v4. Is it still possible to use api v3, or have they shut it down completely?

Can we not manage both side by side based on which version of the library is installed?

@edevenport
Copy link
Contributor Author

@edevenport edevenport commented Dec 16, 2017

@gtmanfred - The ProfitBricks Cloud API v4 implements the much needed image_alias feature. Prior to v4, the image name or UUID was required which both change monthly for many ProfitBricks supplied images. This means the cloud profile can become invalid each month which can cause confusion for users.

Also, while the v3 API is still available, prior versions are not. It's not known when the v3 API will be deprecated. Forcing the currently supported v4 API will prevent this from becoming an issue.

@edevenport edevenport force-pushed the profitbricks_cloudapi_v4 branch from b5dd59f to e0c8c30 Dec 16, 2017
@gtmanfred
Copy link
Contributor

@gtmanfred gtmanfred commented Dec 18, 2017

If it is still available i would rather not force it.

This is the same thing we did with rackspace, we supported their v1 api until they totally deprecated it, and ripped it out then.

@denza
Copy link
Contributor

@denza denza commented Dec 22, 2017

@gtmanfred We added compatibility check for the SDKs where the Profitbricks API 3 is used, like you suggested. This way a user will be informed about possible compatibility problem of a SDK he use. Could you please do a review.

Copy link
Contributor

@gtmanfred gtmanfred left a comment

I left a few stylistic comments, other than that, it looks good! Thanks for making this compatible with the older version!

@denza denza force-pushed the profitbricks_cloudapi_v4 branch from 65b9c46 to f9bb225 Dec 22, 2017
@denza
Copy link
Contributor

@denza denza commented Dec 22, 2017

@gtmanfred Sorry about that. Now we should be good to go.

@denza denza force-pushed the profitbricks_cloudapi_v4 branch from 4939a5d to 3407b27 Dec 22, 2017
@rallytime
Copy link
Contributor

@rallytime rallytime commented Dec 22, 2017

Thank you @edevenport! There are a handful of lint violations here: https://jenkins.saltstack.com/job/PR/job/salt-pr-lint-n/17626/violations/file/salt/cloud/clouds/profitbricks.py/

Can you fix those? This also has a merge conflict as well. Can you resolve that as well?

@denza
Copy link
Contributor

@denza denza commented Dec 25, 2017

@rallytime violations are fixed. Conflict is resolved.

@@ -228,5 +259,5 @@ wait_for_timeout
The timeout to wait in seconds for provisioning resources such as servers.
The default wait_for_timeout is 15 minutes.

For more information concerning cloud profiles, see :ref:`here
<salt-cloud-profiles>`.
For more information concerning cloud profiles, see :doc:`here
Copy link
Contributor

@rallytime rallytime Dec 27, 2017

Is there a reason you changed this reference from using ref to doc? We need to use ref here as using doc should no longer be done. This is failing the following test: https://jenkins.saltstack.com/job/PR/job/salt-pr-linode-ubuntu14-n/18095/testReport/junit/unit.test_doc/DocTestCase/test_check_for_doc_inline_markup/.

@denza
Copy link
Contributor

@denza denza commented Dec 28, 2017

@rallytime I made a fix you proposed for the test. Could you please review?

@rallytime rallytime self-requested a review Dec 30, 2017
@rallytime rallytime merged commit 4d1a324 into saltstack:develop Jan 9, 2018
5 of 9 checks passed
@edevenport edevenport deleted the profitbricks_cloudapi_v4 branch Jan 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants