Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support to authenticate with application credential #1224

Merged
merged 1 commit into from Sep 18, 2018

Conversation

@theopenlab-ci
Copy link

theopenlab-ci bot commented Sep 3, 2018

Build succeeded.

@jtopjian
Copy link
Collaborator

jtopjian commented Sep 3, 2018

@zioproto Travis is flagging a go fmt error. If you can, run go fmt on the changed files - that should fix things up.

@coveralls
Copy link

coveralls commented Sep 4, 2018

Coverage Status

Coverage decreased (-3.4%) to 75.955% when pulling ea7289e on zioproto:application_credential into 4014077 on gophercloud:master.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Sep 4, 2018

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Sep 4, 2018

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Sep 6, 2018

Build succeeded.

@zioproto
Copy link
Contributor Author

zioproto commented Sep 7, 2018

@RdL87 I pushed a new version of the patch that works with any possible combination of ID or NAME env vars. Can you review it ? Are you working on the tests ?

@RdL87
Copy link

RdL87 commented Sep 7, 2018

@zioproto i'm working on this just now. So i'm going to recompile the client with the new library you've just pushed. I will ping you as soon as the keystone becomes available.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Sep 7, 2018

Build succeeded.

@zioproto zioproto changed the title [wip] Add support to authenticate with application credential Add support to authenticate with application credential Sep 14, 2018
@zioproto zioproto force-pushed the application_credential branch 3 times, most recently from c274c69 to 253e20e Compare September 14, 2018 12:42
@zioproto
Copy link
Contributor Author

@jtopjian I believe we are ready for review. I added two tests. I tried to to give correct code pointers to keystone codebase. thank you

@theopenlab-ci
Copy link

theopenlab-ci bot commented Sep 14, 2018

Build succeeded.

Copy link
Collaborator

@jtopjian jtopjian left a comment

Choose a reason for hiding this comment

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

@zioproto This is looking really good. :)

I think I found a good keystone reference to use: https://github.com/openstack/keystoneauth/blob/ebe781a3ea0386d6ff088a84e8dde26e538b856d/keystoneauth1/identity/v3/application_credential.py#L48-L67

It looks to match up really well with your other investigations.

The core functionality of this PR looks correct. The comments I left are just putting some final touches on things.

Let me know if you have any questions :)

auth_options.go Outdated

} else if opts.ApplicationCredentialID != "" {
// Configure the request for ApplicationCredentialID authentication.
// https://github.com/openstack/keystone-specs/blob/master/specs/keystone/queens/application-credentials.rst#using-an-application-credential-to-obtain-a-token
Copy link
Collaborator

Choose a reason for hiding this comment

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

auth_options.go Outdated Show resolved Hide resolved
auth_options.go Show resolved Hide resolved
auth_options.go Outdated Show resolved Hide resolved
auth_options.go Outdated Show resolved Hide resolved
@zioproto
Copy link
Contributor Author

Thanks for the review. I pushed changes that should address all the points of the review

@theopenlab-ci
Copy link

theopenlab-ci bot commented Sep 17, 2018

Build succeeded.

@zioproto
Copy link
Contributor Author

@jtopjian I am not sure what the problem with the coveralls test not passing

@jtopjian
Copy link
Collaborator

@zioproto You can ignore Coveralls. It acts weird sometimes.

Copy link
Collaborator

@jtopjian jtopjian left a comment

Choose a reason for hiding this comment

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

LGTM!

@zioproto @RdL87 Thank you both for your work on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants