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

Modify volume #350

Merged
merged 3 commits into from
Mar 9, 2017
Merged

Modify volume #350

merged 3 commits into from
Mar 9, 2017

Conversation

ehowe
Copy link
Contributor

@ehowe ehowe commented Mar 6, 2017

No description provided.

@geemus
Copy link
Member

geemus commented Mar 6, 2017

Looks like there is a syntax error related to 1.8, which we should kill anyway probably, but fwiw. Otherwise looks good to me. @lanej will leave for you to double check. Thanks!

@ehowe
Copy link
Contributor Author

ehowe commented Mar 6, 2017

@geemus it would be great if we could kill 1.8 support sometime. Either way, that syntax error should be fixed, thanks for pointing it out.

@geemus
Copy link
Member

geemus commented Mar 6, 2017

Agreed, we need to get rid of it. But for now, if they are small changes we should I guess make them just a little longer.

@@ -36,31 +36,50 @@ def ready?
state == 'available'
end

def save
raise Fog::Errors::Error.new('Resaving an existing object may create a duplicate') if persisted?
Copy link
Member

Choose a reason for hiding this comment

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

This case is no longer applicable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, since if there is an identity, its going to call the modify_volume method instead of create_volume.

@@ -63,7 +63,7 @@ def modify_volume(volume_id, options={})
if options['Iops']
volume_modification.merge!(
'originalIops' => volume['iops'],
'targetIops' => options['Iops'],
'targetIops' => options['Iops']
Copy link
Member

Choose a reason for hiding this comment

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

how dare you eugene.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know, right?

@lanej lanej merged commit 4c04799 into fog:master Mar 9, 2017
@geemus
Copy link
Member

geemus commented Mar 9, 2017 via email

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.

3 participants