Expose router feature dataConnectionCount with flag on link create co… #970
Conversation
There was a problem hiding this comment.
This looks mostly fine to me on the whole, apart from the minor issues. However in general I think we would not expect users to need to set this and we should try to set it to the optimal value (or if there are clear trade-offs we should document those clearly and perhaps offer options in terms of those tradeoffs)
api/types/client.go
Outdated
| @@ -17,6 +17,7 @@ type ConnectorCreateOptions struct { | |||
| SkupperNamespace string | |||
| Name string | |||
| Cost int32 | |||
| Dcc int | |||
There was a problem hiding this comment.
Dcc is a bit opaque. Perhaps ConCount or something along those lines?
api/types/types.go
Outdated
| @@ -168,6 +168,7 @@ const ( | |||
| TokenGeneratedBy string = BaseQualifier + "/generated-by" | |||
| SiteVersion string = BaseQualifier + "/site-version" | |||
| TokenCost string = BaseQualifier + "/cost" | |||
| TokenDcc string = BaseQualifier + "/dcc" | |||
There was a problem hiding this comment.
As above, particularly for the actual annotation key, which is user visible, lets have something a bit less cryptic.
cmd/skupper/skupper_link.go
Outdated
| @@ -34,6 +34,7 @@ func NewCmdLinkCreate(skupperClient SkupperLinkClient, flag string) *cobra.Comma | |||
| } | |||
| cmd.Flags().StringVarP(&connectorCreateOpts.Name, flag, "", "", "Provide a specific name for the link (used when deleting it)") | |||
| cmd.Flags().Int32VarP(&connectorCreateOpts.Cost, "cost", "", 1, "Specify a cost for this link.") | |||
| cmd.Flags().IntVarP(&connectorCreateOpts.Dcc, "dataConnectionCount", "", 1, "How many inter-router data connections") | |||
There was a problem hiding this comment.
Other multi word flags use hyphens rather than camel case, so this so follow that pattern for consistency. I think the default should be 0, and that should be treated as unset, in which case a default would be applied by some other part of the code.
pkg/utils/configs/config.go
Outdated
| @@ -38,6 +38,7 @@ connector { | |||
| name: {{.Name}}-connector | |||
| host: {{.Host}} | |||
| port: {{.Port}} | |||
| dataConnectionCount: {{.Dcc}} | |||
There was a problem hiding this comment.
We should only set this if it is non zero. That way we have a way to revert to the router default if desired.
| role: {{.Role}} | ||
| cost: {{.Cost}} | ||
| cost: {{.Cost}} {{if .ConCount}}{{printf "\n dataConnectionCount:" }} {{.ConCount}}{{end}} |
There was a problem hiding this comment.
Noting the comment above, I don't think this is the right way to handle the issue. Use {{- if .ConCount}} ... {{- end}} as in QdRouterConfig() method.
…mmand
This allows user to control data connection count (number of parallel data links) on inter-router connections between skupper sites. -- using a flag on the link create command.
This is my first Skupper PR, somebody please let me know what I missed...