Skip to content
This repository was archived by the owner on Aug 1, 2023. It is now read-only.

Conversation

feiskyer
Copy link
Contributor

No description provided.

@jrperritt
Copy link
Contributor

It looks like you just copy-and-pasted from v1 all the existing operations there. That's not what we want to do. Instead, we should prefer to delegate calls to those operations from v2, similar to the way many operations within the rackspace package delegate to calls within the openstack package (similar to here: https://github.com/rackspace/gophercloud/blob/master/rackspace/networking/v2/networks/delegate.go)

@jrperritt jrperritt changed the title [rfr] Add blockstorage v2 APIs [wip] Add blockstorage v2 APIs Sep 14, 2015
@feiskyer
Copy link
Contributor Author

@jrperritt we cannot proxy those request to v1, because there are some minor differences between the data structure. For example, List API calls different URL and return different json as to v1

@jtopjian
Copy link
Contributor

@feiskyer is right, I think. I was just about to open a PR with my own take on this, but thought I'd see if someone else had gotten to it first. Very close timing :)

Here's my branch: https://github.com/jtopjian/gophercloud/tree/openstack-blockstorage-v2

I split the commits apart by the different BlockStorage features currently in Gophercloud so you can see the differences.

It's interesting that @feiskyer did the same thing as I for the NewBlockStorageV2 function 😄

My branch has a few areas that I've noted that were particularly confusing to me.

With all this said, unit and acceptance tests pass, so it's not horribly off.

@feiskyer feiskyer changed the title [wip] Add blockstorage v2 APIs [rfr] Add blockstorage v2 APIs Sep 15, 2015
@jtopjian
Copy link
Contributor

@jrperritt I did a quick comparison between the requests.go files.

volumetypes seems to have stayed the same between versions, but maybe I missed something.

For volumes and snapshots, there are subtle difference like request parameters and return codes. It looks as though there's potential for a few functions to be reused/delegated, but I think it'd come off as complicating things rather than just being straightforward.

The requests.go files share more similarities. A GetResult that's made up of a commonResult and things like that. But again, I feel that unless a good chunk of the v2 stuff is able to be delegated to the existing v1 stuff, it may just make things tangled and confusing.

I probably just don't understand the approach that you're suggesting well enough.

@feiskyer: Are you able to do a diff on v1 and v2 and recommit? That may make reviewing easier.

Also, did you copy over the acceptance tests, too? I don't see them in the commit, but maybe I missed them. If you'd like I could open a PR with my branch that has the above two items done.

And finally, regarding your notes in #461 about volume attach, IMO, the volume-action stuff is an extension, so it should be in its own package. Unfortunately the BlockStorage API doc doesn't have a separate page for extensions like Nova, so it's hard to tell. IMO, I think each section of that doc should be in its own package. That would follow the existing pattern of volumes, volumetypes, and snapshots.

@jrperritt
Copy link
Contributor

@jtopjian @feiskyer After briefly looking at the v2 API, I think y'all are correct. Perhaps it's naive to try to reuse code between versions of a service. However, due to the size of it, I likely won't get to review it until next week at the earliest, and, even then, probably in chunks.

@jtopjian
Copy link
Contributor

@jrperritt 👍

Would individual PRs for volumes, volumetypes, and snapshots be helpful? Or really just all the same in the end?

@jrperritt
Copy link
Contributor

@jtopjian It's true that in the end it's the same. Of course, splitting them out into separate PRs will be [seemingly] easier to review and individual subservices can be merged more quickly (namely, most users would likely want the volumes functionality more than the others).

@jtopjian
Copy link
Contributor

@jrperritt sounds good.

@feiskyer I don't want to step on your feet or steal your commit. Do you want to break this out into separate PRs? I think having diffs from the original v1 code as well as acceptance tests are needed, too.

@feiskyer
Copy link
Contributor Author

@jrperritt @jtopjian #489 Add volumes part of blockstorage v2 API

@jrperritt By the way, has continuous-integration/travis-ci/pr setted up openstack envs for acceptance test?

@jrperritt
Copy link
Contributor

@feiskyer Travis doesn't run the acceptance tests for 2 reasons:

  1. Time. It'd take hours to complete all of them.
  2. Resources. The acceptance tests use real cloud resources, so there'd have to be a live OpenStack instance to test all this against. Same for Rackspace.

@feiskyer feiskyer closed this Jan 20, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants