search

rackspace / gophercloud

A Go SDK for OpenStack. IN FEATURE FREEZE. See Issue #592
http://gophercloud.io
Other
456 stars 185 forks source link

Keystone Trust added in Extensions #603

Closed codevulture closed 8 years ago

codevulture commented 8 years ago

@jrperritt : Hi, as per discussion on: https://github.com/rackspace/gophercloud/pull/580 i have modified the changes, Please let me know your review points and suggestions.Thanks.

coveralls commented 8 years ago

Coverage Status

Coverage decreased (-0.6%) to 80.455% when pulling 5e8df2ecb23cbb5be1558c2fcc0769a3bc0274fb on codevulture:keystone_api_extension into 985a863d6dd5f928b485dbc8ef440813aafa39ad on rackspace:master.

codevulture commented 8 years ago

@yuanying : Indentation and line changes are resultant of go fmt command which i ran on code , as this was the standard procedure on golang for codeformating like pep in python. So i think this could be ignores or please let me know if needed to be removed.

coveralls commented 8 years ago

Coverage Status

Coverage decreased (-0.3%) to 80.795% when pulling 30235bda7c92722e86bbf859a472b5ef9230b171 on codevulture:keystone_api_extension into 985a863d6dd5f928b485dbc8ef440813aafa39ad on rackspace:master.

coveralls commented 8 years ago

Coverage Status

Coverage decreased (-0.5%) to 80.631% when pulling dd85cdcc17712757935f9b5e867a481cb21109c0 on codevulture:keystone_api_extension into 985a863d6dd5f928b485dbc8ef440813aafa39ad on rackspace:master.

coveralls commented 8 years ago

Coverage Status

Coverage decreased (-0.5%) to 80.631% when pulling dc273fce6936f1608bcfb2f9860c5c3fc02e460a on codevulture:keystone_api_extension into 985a863d6dd5f928b485dbc8ef440813aafa39ad on rackspace:master.

codevulture commented 8 years ago

@jrperritt : Please Review the changes.

codevulture commented 8 years ago

@jrperritt : ping.

jrperritt commented 8 years ago

The go fmt doesn't bother me; in fact, we should be linting and checking for that on build. What bothers me is all of the copy/paste; that shouldn't be necessary. You should convert the opts function parameter to an interface and then have the original AuthOpts and your AuthOpts (AuthOpts plus TrustID) both separately implement the method required by the interface.

codevulture commented 8 years ago

@jrperritt :Thanks for comments,that would mean changing code apart from extensions am i correct.. also I would like to know some more details on how implementations needs to be proceeded to be clear about what needs to be done to have these merged, I have some questions that i would like to ask as i am new to gopherhead library:

I have these files below which have the major code in them, as per your suggestions which files should be changed/removed and if possible can you specify exactly which things, that would help me a lot to implement the changes. like you specified opts but i am not sure which function you referred, please provide your guidance and feedback:

jrperritt commented 8 years ago

There is no reason to have 2 open pull requests for the same feature. Discussion continued in #580