Add load balancer application rule resource and data source #278
Conversation
ghost
added
| return fmt.Errorf("unable to expand load balancer app rule: %s", err) | ||
| } | ||
|
|
||
| createdPool, err := edgeGateway.CreateLBAppRule(LBRule) |
There was a problem hiding this comment.
CreateLBAppRule(LBRule) - IMHO would be much better readable as CreateLBAppRule(name, script). as it's now it shows that LBRULE ir complex, but it isn't
There was a problem hiding this comment.
I could create wrapper, but I would like to keep this consistent and transparent to the struct we have in types.
The reason I dislike when we introduce our own "simplified types" for structs is that we end up translating between structs. Then if a new field appears (or we missed some) in the configuration our APIs become deprecated and a new method must be created.
Again if this is a team decision I can change :)
@lvirbalas, @dataclouder, your thoughts about it?
There was a problem hiding this comment.
One more thought. What if I added more explanation to the type fields we have in Go fashion. That way any modern IDE would show more context of what is expected in the field but would still keep the transparency.
These are mockups comments, not exactly the ones I intend to add.
There was a problem hiding this comment.
The reason I dislike when we introduce our own "simplified types" for structs is that we end up translating between structs. Then if a new field appears (or we missed some) in the configuration our APIs become deprecated and a new method must be created.
Again if this is a team decision I can change :)
I dislike the simplified types as well, as they add another layer of abstraction.
There was a problem hiding this comment.
Well, seems we have diff reading and API usage experience. For me as terraform and API reader/user it's easier to pass two string, than Structure with four fields which I have to check which to fill. From Java world there is rule if method with 4-5 parameters when refactor to pass object.
There was a problem hiding this comment.
It may be so. But I have already burnt with such methods in this project. I had to change quite a few details on VM networking just because there were "simplified" fields missing important details. The public API gets too many changes this way.
There was a problem hiding this comment.
In the light of current discussion - what if I add a wrapper for it now and we have both options available?
There was a problem hiding this comment.
I think you already have this approach for all other previous LB resources. Also, there are many other instances of this approach in EG already. Hence, I suggest leaving this as is in this PR, but @dataclouder could add this to refactoring consideration together with vmware/go-vcloud-director#211
There was a problem hiding this comment.
I am OK with @Didainius approach.
Simplified structures should be used only when: (I) we have more than 3 fields to pass by position, (II) the original structures are not flat, (III) require internal structure manipulation, (IV) and can't be passed to update methods without changes. A clear example is Edge Gateway, where the structure contains a huge amount of details that we may not intend to deal with in every operation. But this structure is manageable as it is.
| return fmt.Errorf("unable to expand load balancer app rule: %s", err) | ||
| } | ||
|
|
||
| createdPool, err := edgeGateway.CreateLBAppRule(LBRule) |
There was a problem hiding this comment.
I am OK with @Didainius approach.
Simplified structures should be used only when: (I) we have more than 3 fields to pass by position, (II) the original structures are not flat, (III) require internal structure manipulation, (IV) and can't be passed to update methods without changes. A clear example is Edge Gateway, where the structure contains a huge amount of details that we may not intend to deal with in every operation. But this structure is manageable as it is.
Continuing work on terraform-providers/terraform-provider-vcd#223
This one introduces resource and data source for Load Balancer Application Rule.