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

fix: attach volume on #save, remove #server= #443

Merged
merged 2 commits into from
Apr 23, 2018
Merged

Conversation

lanej
Copy link
Member

@lanej lanej commented Apr 19, 2018

s/service/server/ in Volume#save. Fixes attaching after initialization.

Require users to call Volume#attach(server, device) to attach a volume after it's created. Volume#server=(new_server) having a side-effect of attaching a volume is confusing.

Volume#device set on Volume#initialize will be lost when calling Volume#wait_for in fog-core ~> 2.0.

BREAKING CHANGE

Volume#server= is removed.

s/service/server/ in Volume#save.  Fixes attaching after initialization.

Require users to call #attach(server, device) to attach a volume after
it's created.  Volume#server=(new_server) having a side-effect of
attaching a volume is confusing.

Volume#device set on #initialize will be lost when calling
Volume#wait_for in fog-core ~> 2.0.

BREAKING CHANGE

Volume#server= is removed.
@lanej lanej requested a review from geemus April 19, 2018 17:10
@lanej
Copy link
Member Author

lanej commented Apr 19, 2018

 FOG_MOCK=false bundle exec shindo tests/models/compute/volume_tests.rb
[fog][WARNING] Setting default fog timeout to 2000 seconds

  Fog::Compute[:aws] | volume (aws)
    success
      #save + succeeds
      attached + succeeds
      #detach + succeeds
      #attach(server, device) + succeeds
      #force_detach + succeeds
+ returns true
+ returns "io1"
+ returns 5000
+ returns 100
      @instance.tags + returns {"key"=>"value"}
      #destroy + succeeds

  11 succeeded in 385.32015 seconds

Coverage report generated for Shindo to /Users/jlane/p/fog-aws/coverage. 2849 / 6718 LOC (42.41%) covered.

Copy link
Member

@geemus geemus left a comment

Choose a reason for hiding this comment

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

Looks good, as per previous discussion.

The only question I would still have is, is there a reasonable way for us to warn people more directly if they are expecting the old usage? I'm not sure, but if we can warn that would be good, so I welcome your thoughts.

@lanej
Copy link
Member Author

lanej commented Apr 19, 2018

The old usage would require someone to use Volume#server= which will get them a NoMethodError (as of this change).

This is certainly a major bump. Not sure what Fog considers best practices in this case.

@geemus
Copy link
Member

geemus commented Apr 20, 2018

Maybe we could define a Volume#server= in order to have it raise a more explanatory error message? We haven't really had this situation enough to have a policy per se. Usually I've tried to just deprecate and then eventually drop, rather than just dropping functionality. But that doesn't seem very doable here.

@lanej
Copy link
Member Author

lanej commented Apr 20, 2018

Deprecation is not an option in this case.

I will add Volume#server= back in with a message leading the user to Volume#attach

@geemus
Copy link
Member

geemus commented Apr 20, 2018

@lanej yeah, I was pretty sure deprecation wasn't possible unless I was totally missing something. I think the more explicit error message sounds like a great just-in-case. Thanks!

@geemus
Copy link
Member

geemus commented Apr 23, 2018

Thanks for the extra care/messaging. LGTM.

@lanej lanej merged commit e0447b0 into master Apr 23, 2018
@lanej lanej deleted the lanej/fix-volume-attach branch April 23, 2018 16:31
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