Skip to content

Identity V3: Add credentials package (#1457)#1460

Merged
jtopjian merged 1 commit intogophercloud:masterfrom
himanshuvar:master
Mar 3, 2019
Merged

Identity V3: Add credentials package (#1457)#1460
jtopjian merged 1 commit intogophercloud:masterfrom
himanshuvar:master

Conversation

@himanshuvar
Copy link
Copy Markdown
Contributor

@himanshuvar himanshuvar commented Feb 20, 2019 •
edited by jtopjian
Loading

  • Identity V3: Add credentials package

For #1457

Adding the support for Keystone Credential package:

https://github.com/openstack/keystone/tree/master/keystone/credential

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Feb 20, 2019

Build failed.

@coveralls
Copy link
Copy Markdown

coveralls commented Feb 21, 2019 •
edited
Loading

Coverage Status

Coverage increased (+0.02%) to 76.547% when pulling b482e8a on himanshuvar:master into cfa8434 on gophercloud:master.

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Feb 21, 2019

Build failed.

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Feb 27, 2019

Build succeeded.

kayrus
kayrus reviewed Feb 27, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@kayrus kayrus left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution. It would be great if you also add acceptance tests.

@kayrus kayrus mentioned this pull request Feb 27, 2019
Merged
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Feb 28, 2019

Build succeeded.

kayrus
kayrus requested changes Feb 28, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@kayrus kayrus left a comment

Choose a reason for hiding this comment

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

omitempty is required, otherwise Openstack API will get an empty string and fail.

kayrus
kayrus reviewed Feb 28, 2019
View reviewed changes
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Feb 28, 2019

Build succeeded.

@himanshuvar
Identity V3: Add credentials package (gophercloud#1457)
b482e8a 
  * Add credentials package.

  * Addressed comments.

  * Add credential acceptance test.
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Feb 28, 2019

Build succeeded.

@ghost ghost mentioned this pull request Mar 1, 2019
Merged
@jtopjian
Copy link
Copy Markdown
Contributor

jtopjian commented Mar 1, 2019

@himanshuvar Thank you for working on this. @kayrus Thank you for looking it over, too!

Would it be better to have Blob as map[string]interface{}?

I think it should be possible to convert it to a marshalled string in ToCredentialCreateMap and unmarshall it in a Credential UnmarshalJSON method without too much difficulty.

Thoughts?

@kayrus
Copy link
Copy Markdown
Contributor

kayrus commented Mar 1, 2019

@jtopjian , unfortunately no, since blob can handle also a cert, or any unstructured string data.

@jtopjian
Copy link
Copy Markdown
Contributor

jtopjian commented Mar 1, 2019

Ah, understood. Thanks!

@jtopjian
Copy link
Copy Markdown
Contributor

jtopjian commented Mar 1, 2019

@himanshuvar Is this ready for review/merge?

@himanshuvar
Copy link
Copy Markdown
Contributor Author

himanshuvar commented Mar 2, 2019

eview/merge?
Yes

jtopjian
jtopjian approved these changes Mar 3, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

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

@himanshuvar Thank you for submitting this.

@kayrus Thank you for the detailed review.

@jtopjian jtopjian merged commit a6b91de into gophercloud:master Mar 3, 2019
@jtopjian jtopjian mentioned this pull request Mar 3, 2019
Closed
@kayrus kayrus mentioned this pull request Mar 22, 2020
Merged
@kayrus kayrus mentioned this pull request Mar 31, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jtopjian jtopjian jtopjian approved these changes

@kayrus kayrus kayrus approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@himanshuvar @coveralls @jtopjian @kayrus