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

Keystone Trust added in Extensions #603

Closed
wants to merge 1 commit into from
Closed

Keystone Trust added in Extensions #603

wants to merge 1 commit into from

Conversation

codevulture
Copy link
Contributor

@jrperritt : Hi, as per discussion on: #580 i have modified the changes, Please let me know your review points and suggestions.Thanks.

@@ -44,7 +44,7 @@ type AuthOptions struct {
// possible. This setting defaults to false.
//
// NOTE: The reauth function will try to re-authenticate endlessly if left unchecked.
// The way to limit the number of attempts is to provide a custom HTTP client to the provider client
Copy link

Choose a reason for hiding this comment

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

Seems unneeded change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

go fmt result.

@coveralls
Copy link

coveralls commented Jul 4, 2016

Coverage Status

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

@codevulture
Copy link
Contributor Author

@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
Copy link

coveralls commented Jul 4, 2016

Coverage Status

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

}
} else {
// Password authentication.
Copy link

Choose a reason for hiding this comment

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

Why did you remove password authentication? We can use this if we provide trustID.

@coveralls
Copy link

coveralls commented Jul 6, 2016

Coverage Status

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

@coveralls
Copy link

coveralls commented Jul 6, 2016

Coverage Status

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

@codevulture
Copy link
Contributor Author

@jrperritt : Please Review the changes.

@codevulture
Copy link
Contributor Author

@jrperritt : ping.

@jrperritt
Copy link
Contributor

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
Copy link
Contributor Author

codevulture commented Jul 14, 2016

@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:

  • openstack/identity/v3/extensions/auth_options.go
  • openstack/identity/v3/extensions/tokens/endpoint_location.go
  • openstack/identity/v3/extensions/tokens/requests.go
  • openstack/identity/v3/extensions/tokens/requests_test.go
  • openstack/identity/v3/extensions/tokens/results.go
  • openstack/identity/v3/extensions/tokens/urls.go
  • openstack/identity/v3/extensions/trust/request.go

@jrperritt
Copy link
Contributor

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

@jrperritt jrperritt closed this Jul 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.

4 participants