Skip to content

Support http_basic auth_strategy for Ironic#1983

Merged
jtopjian merged 1 commit intogophercloud:masterfrom
iurygregory:ironic-http_basic
Jun 24, 2020
Merged

Support http_basic auth_strategy for Ironic#1983
jtopjian merged 1 commit intogophercloud:masterfrom
iurygregory:ironic-http_basic

Conversation

@iurygregory
Copy link
Copy Markdown
Contributor

@iurygregory iurygregory commented Jun 22, 2020 •
edited
Loading

For #1429

This PR adds the support to use Ironic when auth_strategy is http_basic.

Ironic introduced a new auth_type http_basic that can
be used in standalone deployments[1].

This commit adds the support to use the new authentication
type.

The code was introduced by the changes [2] and [3].

[1] https://docs.openstack.org/ironic/latest/install/standalone.html
[2] https://review.opendev.org/727467
[3] https://review.opendev.org/729070

@iurygregory iurygregory force-pushed the ironic-http_basic branch 3 times, most recently from da2cf0a to b44070b Compare June 22, 2020 22:51
@coveralls
Copy link
Copy Markdown

coveralls commented Jun 22, 2020 •
edited
Loading

Coverage Status

Coverage increased (+0.02%) to 79.398% when pulling 141aaa8 on iurygregory:ironic-http_basic into 8b8b153 on gophercloud:master.

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Jun 23, 2020

Build succeeded.

jtopjian
jtopjian requested changes Jun 23, 2020
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.

@iurygregory Thank you for submitting this!

I left one doc nit. The only reason it's important is because these docs get generated to display on godoc.org.

In addition, can you change all occurrences of Http to HTTP? This will help match Gophercloud's coding style. If http is all lowercase to begin with, it can stay http. For example:

httpClient := httpbasic.NewBareMetalHTTPBasic()

With regard to the acceptance tests, per our email conversation, you can omit them, but given you've submitted a really good set of tests, a better option might be to add a new conditional here (https://github.com/gophercloud/gophercloud/blob/master/acceptance/clients/conditions.go) so that these tests are only run when a certain environment variable is set.

And finally, per our contributor's guide, we need to also reference the service-side code that implements this functionality.

Please let me know if you have any questions. Thank you again.

@iurygregory
Copy link
Copy Markdown
Contributor Author

iurygregory commented Jun 23, 2020 •
edited
Loading

@iurygregory Thank you for submitting this!

I left one doc nit. The only reason it's important is because these docs get generated to display on godoc.org.

In addition, can you change all occurrences of Http to HTTP? This will help match Gophercloud's coding style. If http is all lowercase to begin with, it can stay http. For example:

Sure!

httpClient := httpbasic.NewBareMetalHTTPBasic()

With regard to the acceptance tests, per our email conversation, you can omit them, but given you've submitted a really good set of tests, a better option might be to add a new conditional here (https://github.com/gophercloud/gophercloud/blob/master/acceptance/clients/conditions.go) so that these tests are only run when a certain environment variable is set.

I noticed that we only run the tests from the v1, according to the shell script.
I will add the changes to conditions.go also, just wondering if anything else is required.

And finally, per our contributor's guide, we need to also reference the service-side code that implements this functionality.

Should be enough add the to add the links for the code changes in ironic in the PR description?
https://review.opendev.org/727467
https://review.opendev.org/729070

Please let me know if you have any questions. Thank you again.

@iurygregory iurygregory force-pushed the ironic-http_basic branch 2 times, most recently from 83f3537 to 47bc219 Compare June 23, 2020 14:51
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Jun 23, 2020

Build failed.

@jtopjian
Copy link
Copy Markdown
Contributor

jtopjian commented Jun 23, 2020

I will add the changes to conditions.go also, just wondering if anything else is required.

You'll need to add the conditional check to the beginning of each test. For example:

gophercloud/acceptance/openstack/compute/v2/hypervisors_test.go

Line 17 in 737db56

clients.RequireAdmin(t)

Should be enough add the to add the links for the code changes in ironic in the PR description?

That's perfect - thank you!

@iurygregory iurygregory force-pushed the ironic-http_basic branch 2 times, most recently from a921b61 to f0d32a9 Compare June 23, 2020 18:14
@iurygregory
Copy link
Copy Markdown
Contributor Author

iurygregory commented Jun 23, 2020

clients.

@iurygregory iurygregory reopened this Jun 23, 2020
@iurygregory
Copy link
Copy Markdown
Contributor Author

iurygregory commented Jun 23, 2020

I will add the changes to conditions.go also, just wondering if anything else is required.

You'll need to add the conditional check to the beginning of each test. For example:

gophercloud/acceptance/openstack/compute/v2/hypervisors_test.go

Line 17 in 737db56

clients.RequireAdmin(t)

Should be enough add the to add the links for the code changes in ironic in the PR description?

That's perfect - thank you!

Done =) Thanks! (sorry I clicked the wrong button and closed the PR)

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Jun 23, 2020

Build succeeded.

jtopjian
jtopjian requested changes Jun 24, 2020
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.

Two small changes and I think this is good to go.

Support http_basic auth_strategy for Ironic
141aaa8 
Ironic introduced a new auth_type `http_basic` that can
be used in standalone deployments[1]. The `http_basic`

This commit adds the support to use the new authentication
type.

[1] https://docs.openstack.org/ironic/latest/install/standalone.html
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Jun 24, 2020

Build succeeded.

jtopjian
jtopjian approved these changes Jun 24, 2020
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 - thank you!

@jtopjian jtopjian merged commit 1f7127e into gophercloud:master Jun 24, 2020
@stbenjam stbenjam mentioned this pull request Apr 20, 2022
Open
61 tasks
@iurygregory iurygregory deleted the ironic-http_basic branch October 13, 2023 20:49
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

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@iurygregory @coveralls @jtopjian