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

[salt-cloud] Fix creating droplet from snapshot in digital_ocean provider #26824

Merged
merged 3 commits into from Sep 2, 2015
Merged

[salt-cloud] Fix creating droplet from snapshot in digital_ocean provider #26824

merged 3 commits into from Sep 2, 2015

Conversation

systembell
Copy link
Contributor

Problem
Digital Ocean's API returns 'None' instead of None for a non-existent image slug, rendering the conditional in get_image useless:

[DEBUG   ] Sending event - data = {'_stamp': '2015-09-01T19:22:48.162645', 'event': 'requesting instance', 'kwargs': {'ssh_keys': ['XXXXXXX], 'region': 'nyc3', 'ipv6': True, 'private_networking': True, 'backups': False, 'size': '1gb', 'image': 'None', 'name': 'test1'}}
[ERROR   ] Error creating salt-test1 on DIGITAL_OCEAN

The following exception was thrown when trying to run the initial deployment: An error occurred while querying DigitalOcean. HTTP Code: 422  Error: u'{"id":"unprocessable_entity","message":"You specified an invalid image for Droplet creation."}'

Solution
if image['slug'] != 'None'. Fixes #22724.

@techhat
Copy link
Contributor

techhat commented Sep 1, 2015

@Pravka this makes sense, but somebody is going to come along and change it back if there's no comment explaining why it's like this. Could you please add a comment?

@jfindlay jfindlay added Salt-Cloud pending-changes The pull request needs additional changes before it can be merged RIoT Relates to integration with cloud providers, hypervisors, API-based services, etc. Minor Change labels Sep 1, 2015
@systembell
Copy link
Contributor Author

@techhat Thanks -- I dug a little deeper and noticed that the real issue is that digital_ocean_v2.avail_images is converting all the values in each image dict (including None) to a string; this was probably not the desired result (the types in the response from Digital Ocean's API should remain the same, especially constants).

So, I've reverted my initial fix and updated avail_images and get_image to fix this behavior.

tl;dr (for those who might come after me and want to change it): changing None to a string (unless it's '') is bad, we shouldn't do it.

@systembell systembell changed the title [salt-cloud] Fix creating droplet from snapshots in digital_ocean provider [salt-cloud] Fix creating droplet from snapshot in digital_ocean provider Sep 2, 2015
techhat added a commit that referenced this pull request Sep 2, 2015
…hot-in-dov2

[salt-cloud] Fix creating droplet from snapshot in digital_ocean provider
@techhat techhat merged commit cdc0ea2 into saltstack:2015.5 Sep 2, 2015
@techhat
Copy link
Contributor

techhat commented Sep 2, 2015

I like it. Thanks @Pravka!

@systembell
Copy link
Contributor Author

FYI @techhat, 2015.8 and develop are also affected by this; I'm assuming that this didn't happen automatically because digital_ocean_v2 was moved to digital_ocean after 2015.5.5. Do you want me to open a separate PR for 2015.8?

@techhat
Copy link
Contributor

techhat commented Sep 2, 2015

@Pravka this will get merged forward into 2015.8 and develop, probably in the next day or so, so don't worry about it. Ping @basepi.

@systembell
Copy link
Contributor Author

Thanks!

@systembell
Copy link
Contributor Author

This still has not been merged into 2015.8 or develop.

@rallytime
Copy link
Contributor

@Pravka The change should be in 2015.5, as that is where you made the pull request. It looks correct to me here and here - Sorry, I thought said 2015.5 originally, but you said 2015.8. My mistake. :)

However, I think your original concern was correct about the merge forward. I've ported your change to the digital_ocean.py driver in 2015.8 in #27164. Once that is merged, it should merge-forward cleanly into develop.

rallytime pushed a commit that referenced this pull request Sep 16, 2015
Make sure changes from #26824 to digital_ocean_v2.py driver make it to digital_ocean.py in 2015.8
@basepi
Copy link
Contributor

basepi commented Sep 16, 2015

Ah, yes, I think I saw these changes but since v2 doesn't exist in 2015.8 I disregarded those changes on the merge forward.

@rallytime for my information, is 2015.5's digital_ocean_v2 the same as 2015.8's digital_ocean? Was it renamed when the old one was deprecated?

@systembell systembell deleted the fix-droplet-creation-from-snapshot-in-dov2 branch September 16, 2015 19:30
@rallytime
Copy link
Contributor

@basepi Yep. D.O. is removing support for their original API Nov 1st. So I removed the original driver (based on APIv1) and renamed digital_ocean_v2.py to just digital_ocean.py starting with 2015.8.0.

@basepi
Copy link
Contributor

basepi commented Sep 16, 2015

Thanks for the heads up. I've probably lost a few fixes on merge forward, unfortunately.

@rallytime
Copy link
Contributor

Yeah I was just thinking that actually. I will see what I can do to get any changes in that driver merged forward today.

@basepi
Copy link
Contributor

basepi commented Sep 16, 2015

If it hasn't changed substantially in 2015.8 you may be able to just eyeball a diff of the two files and bring over the new stuff from 2015.5. Sorry to make your life difficult.

@systembell
Copy link
Contributor Author

avail_images and get_image are the affected functions. I can open a PR for 2015.8/develop if y'all want.

@rallytime
Copy link
Contributor

@Pravka I already did for this particular pull request. It's been merged into the 2015.8 branch in PR #27164. :)

What @basepi and I were talking about was if there were other changes to this driver in the 2015.5 branch that didn't make it into the 2015.8 for the same reason that this one didn't. I just didn't even think about it. So, @Pravka I think we're good as far as your change here goes. Thanks again for the fix!

@systembell
Copy link
Contributor Author

Awesome. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pending-changes The pull request needs additional changes before it can be merged RIoT Relates to integration with cloud providers, hypervisors, API-based services, etc. Salt-Cloud
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants