Skip to content

Conversation

@auhlig
Copy link
Member

@auhlig auhlig commented May 18, 2016

As discussed in #666 this introduces the BuildersV2 and BuildersV3 to make it less likely to get confused by the 2 different Identity versions.

What do you think @gondor , @dhague ?

@auhlig auhlig added this to the 3.0.1 Release milestone May 18, 2016
@dhague
Copy link
Collaborator

dhague commented May 18, 2016

As discussed with @auhlig via PM just now, this idea of using Builders.<object>() for everything except Identity, where we use BuilderV2.<object>() and BuilderV3.<object>() doesn't feel quite right.
A better approach might be to stay with Builders.<object>() which would always use the latest version of an API (and a point release of OS4J whenever this changes), together with Builders.<service>().<object>() when a specific version is required.
This would give, for example:
Builders.user() creating a V3 user (in the current 3.0 release of OS4J)
Builders.identityV3().user() creating a V3 user regardless of OS4J release (>3.0)
Builders.identityV2().user() creating a V2 user regardless of OS4J release (>3.0)

This same pattern can then be used to support different versions of the other OpenStack services, and has the advantage that users of OS4J v3.0 would not have a breaking change when using V3 user objects.

@gondor
Copy link
Member

gondor commented May 18, 2016

@auhlig @dhague I agree with the Builders.<service>.<object> approach. I was thinking about it when I was getting ready for work and have V2 and V3 builders felt odd to me and would impose lots of breaking changes in the future. I like rooting it to the service based on the version of the service being used. Well promote less breaking changes.

@vinodborole
Copy link
Contributor

vinodborole commented May 18, 2016

Builders.<service>.<object>
makes more sense

@gondor
Copy link
Member

gondor commented May 18, 2016

@auhlig @dhague - I know this is a PR :) But wanted to introduce @vinodborole who made a comment above. He's been a big contributor as well. Do you mind also reviewing his latest PR which is targeted for V3 - PR #671 . @vinodborole - Darren and Arno did the V3 work, infact built the whole 3.0.0 release.

@vinodborole
Copy link
Contributor

@gondor Thanks for introducing me to the team; its a privilege working with you guyz. Let me know what you think about my PR;

@wumingcheng
Copy link

@auhlig when does the new version publish, two weaks has gone.And don't forget to amend the bug of #679,

@gondor
Copy link
Member

gondor commented May 26, 2016

Currently its done manually by me. Working on integrating merged changes to deploy new snapshots via Travis CI.

@auhlig
Copy link
Member Author

auhlig commented May 26, 2016

Hey @gondor, @dhague, @vinodborole,

I added commit b269507 implementing what we discussed. I also extended that pattern to the other services. Comes with groovy tests. Let me know what you think.

@vinodborole
Copy link
Contributor

LGTM

@gondor
Copy link
Member

gondor commented May 26, 2016

LGTM - great job!

@auhlig auhlig merged commit 0937b89 into ContainX:master May 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants