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 load balancer server pool CRUD #205

Merged
merged 9 commits into from
Jul 1, 2019
Merged

Conversation

Didainius
Copy link
Collaborator

Part of implementing edge gateway load balancers https://github.com/terraform-providers/terraform-provider-vcd/issues/223

This PR introduces load balancer server pool support in govcd.

Copy link
Collaborator

@lvirbalas lvirbalas left a comment

Choose a reason for hiding this comment

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

Great to see this coming! First pass feedback inline.

Copy link
Contributor

@dataclouder dataclouder left a comment

Choose a reason for hiding this comment

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

Some preliminary checks. More review coming

Copy link
Contributor

@dataclouder dataclouder left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@lvirbalas lvirbalas left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@vbauzys vbauzys left a comment

Choose a reason for hiding this comment

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

A few things to discuss before approval.

// Update - change a single field and compare that configuration and result objects are deeply equal
// Update - try and fail to update without mandatory field
// Delete
func (vcd *TestVCD) Test_LBServerPool(check *C) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not fond of such unit test where all functionality tested. It more like workflow test and not function unit test. @dataclouder

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I could split them, however this would make total run time longer due to for situations such as Test_ReadLBServerPool, Test_UpdateLBServerPool, Test_DeleteLBServerPool I would still need to create for each scenario.

Copy link
Contributor

@vbauzys vbauzys left a comment

Choose a reason for hiding this comment

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

After @dataclouder accepts such test, then PR can be merged.

@Didainius Didainius self-assigned this Jul 1, 2019
@Didainius Didainius merged commit e396efd into vmware:master Jul 1, 2019
@Didainius Didainius deleted the lb22 branch July 1, 2019 08:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants