-
Notifications
You must be signed in to change notification settings - Fork 180
Extension: Disk Config #256
Conversation
@jrperritt, @jamiehannaford: This is ready for a review! It's also a good example of what I had in mind for the Rackspace compute provider: I've implemented |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add in the testing.Short()
bit here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added from your branch in the rebase 😉
These are helpful for testing .ToXyzMap() methods, in particular.
2856cc4
to
5d68672
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In several of the other services, we just have Extract
. I know that there is one that doesn't fit that mold (ExtractDiskConfig
) but for consistency, we should either change those to reflect this way, or vice versa.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an extension, though. I was following @jamiehannaford's lead on naming here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, ok. I'm fine with having it like that. I'm just thinking that having 2 ways of writing an extraction will force the users to know if they're using an extension or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm inclined to agree. The logic is that you can perform one Get
call, stash the GetResult
, and use different Extract
functions to pull data relevant to different extensions from it without having to make additional calls.
I think the extension mechanisms we're using need a little work, still - it's awkward to compose extensions and it's all a little too manual right now. I'm not sure what I'd rather see, though, and I think it'll need to wait for post-release. I'll try to articulate my thoughts about what I'd like to accomplish in a discussion issue, possibly after these next two weeks are over and we have 1.0 shipped 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Any changes can wait until a later release.
Waiting for green... |
and there we go 🚢 |
Changes Unknown when pulling 9e87a92 on smashwilson:disk-config into * on rackspace:v0.2.0*. |
Implementing the disk-config extension for OpenStack and Rackspace compute.