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 providers support to openstack loadbalancer #1765

Merged
merged 1 commit into from Nov 22, 2019

Conversation

luis5tb
Copy link
Contributor

@luis5tb luis5tb commented Nov 8, 2019

@luis5tb luis5tb changed the title Add providers support to openstack loadbalancer [WIP] Add providers support to openstack loadbalancer Nov 8, 2019
@theopenlab-ci
Copy link

theopenlab-ci bot commented Nov 8, 2019

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Nov 8, 2019

Build succeeded.

@luis5tb luis5tb changed the title [WIP] Add providers support to openstack loadbalancer Add providers support to openstack loadbalancer Nov 19, 2019
@luis5tb luis5tb force-pushed the lbs-providers-support branch 3 times, most recently from d84825d to 6ce0472 Compare November 19, 2019 12:26
@coveralls
Copy link

coveralls commented Nov 19, 2019

Coverage Status

Coverage decreased (-0.8%) to 76.2% when pulling 4e3a521 on luis5tb:lbs-providers-support into bee6224 on gophercloud:master.

Copy link
Contributor

@dulek dulek left a comment

Choose a reason for hiding this comment

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

Looks fine to me!

Copy link
Contributor

@MaysaMacedo MaysaMacedo left a comment

Choose a reason for hiding this comment

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

Looks good.

@jtopjian
Copy link
Contributor

All, thank you for your collaboration with this work and submitting it.

Please note that we have a contributor tutorial which will explain additional steps required before we can merge this PR.

Please let me know if you have any questions.

@luis5tb
Copy link
Contributor Author

luis5tb commented Nov 19, 2019

All, thank you for your collaboration with this work and submitting it.

Please note that we have a contributor tutorial which will explain additional steps required before we can merge this PR.

Please let me know if you have any questions.

Thanks! I suppose I need to open an issue and properly format the PR commit message! Let me know if something else is missing

@jtopjian
Copy link
Contributor

We'll also need to see the actual Python API implementation that "proves" this feature (https://github.com/gophercloud/gophercloud/blob/master/docs/contributor-tutorial/step-03-code-hunting.md).

Acceptance tests (https://github.com/gophercloud/gophercloud/blob/master/docs/contributor-tutorial/step-04-acceptance-testing.md) are also important to include, but if it's not possible or too difficult to include them, we can make exceptions.

@luis5tb
Copy link
Contributor Author

luis5tb commented Nov 19, 2019

We'll also need to see the actual Python API implementation that "proves" this feature (https://github.com/gophercloud/gophercloud/blob/master/docs/contributor-tutorial/step-03-code-hunting.md).

Acceptance tests (https://github.com/gophercloud/gophercloud/blob/master/docs/contributor-tutorial/step-04-acceptance-testing.md) are also important to include, but if it's not possible or too difficult to include them, we can make exceptions.

Sure! Already added the issue and updated the commit message with it and pointer to octavia providers api. As for the acceptance tests, I've already tested it, in conjunction with Cluster Network Operator making use of it: openshift/cluster-network-operator#391

@jtopjian
Copy link
Contributor

Already added the issue and updated the commit message with it and pointer to octavia providers api

Thanks, but API documentation isn't what we're looking for. We need to see the actual Python service API implementation as described here:

https://github.com/gophercloud/gophercloud/blob/master/docs/contributor-tutorial/step-03-code-hunting.md

Specifically, see the area that reads "Examples of good code hunting can be seen here:".

As for the acceptance tests, I've already tested it,

Is it possible to add that test here?

https://github.com/gophercloud/gophercloud/tree/master/acceptance/openstack/loadbalancer/v2

@luis5tb
Copy link
Contributor Author

luis5tb commented Nov 19, 2019

Already added the issue and updated the commit message with it and pointer to octavia providers api

Thanks, but API documentation isn't what we're looking for. We need to see the actual Python service API implementation as described here:

https://github.com/gophercloud/gophercloud/blob/master/docs/contributor-tutorial/step-03-code-hunting.md

Specifically, see the area that reads "Examples of good code hunting can be seen here:".

Done!

As for the acceptance tests, I've already tested it,

Is it possible to add that test here?

https://github.com/gophercloud/gophercloud/tree/master/acceptance/openstack/loadbalancer/v2

I'll check! Thanks!

@theopenlab-ci
Copy link

theopenlab-ci bot commented Nov 19, 2019

Build succeeded.

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

@luis5tb Thanks again for your work on this. I've left a comment regarding the Fields option that List supports. And per previous discussion, let me know if you have any issues including an acceptance test.

openstack/loadbalancer/v2/providers/requests.go Outdated Show resolved Hide resolved
@luis5tb
Copy link
Contributor Author

luis5tb commented Nov 20, 2019

@luis5tb Thanks again for your work on this. I've left a comment regarding the Fields option that List supports. And per previous discussion, let me know if you have any issues including an acceptance test.

Thanks for your reviews! I'll give it a try later today to the acceptance tests! I'll let you know if I'm hitting any problem

@theopenlab-ci
Copy link

theopenlab-ci bot commented Nov 20, 2019

Build succeeded.

@jtopjian
Copy link
Contributor

@luis5tb Thank you for the updates. I should be able to review this again within the next day.

Copy link
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 and everyone else for working on this.

@jtopjian jtopjian merged commit 2567326 into gophercloud:master Nov 22, 2019
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

5 participants