govc: Add support for getting the VC proxy and no-proxy configuration #2435
Conversation
Introduced the vAPI bindings and the implementation for those
govc output:
```
HTTP proxy: Enabled
Server: http://10.186.17.218
Port: 80
Username: user
HTTPS proxy: Disabled
FTP proxy: Enabled
Server: http://google.com
Port: 21
No Proxy addresses: localhost, 127.0.0.1
```
govc/vcsa/proxy/proxy.go
Outdated
| limitations under the License. | ||
| */ | ||
|
|
||
| package logging |
There was a problem hiding this comment.
should be package networking or package net (see comment below)
govc/vcsa/proxy/proxy.go
Outdated
| @@ -0,0 +1,119 @@ | |||
| /* | |||
| Copyright (c) 2020 VMware, Inc. All Rights Reserved. | |||
govc/vcsa/proxy/proxy.go
Outdated
| } | ||
|
|
||
| func init() { | ||
| cli.Register("vcsa.networking.proxy.info", &info{}) |
There was a problem hiding this comment.
Similar to how we named vcsa.log.* instead of vcsa.logging.*, can we use vcsa.net.* as the prefix here?
govc/vcsa/proxy/proxy.go
Outdated
| var res proxyResult | ||
| res.proxy = proxyRes | ||
| res.noProxy = noProxyRes | ||
| return cmd.WriteResult(res) |
There was a problem hiding this comment.
You can collapse these 4 lines into:
return cmd.WriteResult(&proxyResult{proxyRes, noProxyRes})
vapi/appliance/networking/proxy.go
Outdated
| } | ||
|
|
||
| // ProtocolProxyConfig represents configuration for specific proxy - ftp, http, https. | ||
| type ProtocolProxyConfig struct { |
vapi/appliance/networking/proxy.go
Outdated
| } | ||
|
|
||
| // Proxy returns all Proxy configuration. | ||
| func (m *Manager) Proxy(ctx context.Context) (ProxyConfig, error) { |
There was a problem hiding this comment.
Let's name this method ProxyConfig. Based on the API, I think a Proxy method would be for a specific protocol such as /rest/appliance/networking/proxy/https
We also normally return a pointer, *ProxyConfig in this case.
There was a problem hiding this comment.
Thanks, renamed the NoProxy to NoProxyConfig as well
vapi/appliance/networking/proxy.go
Outdated
| } | ||
|
|
||
| for _, c := range rawRes { | ||
| switch { |
There was a problem hiding this comment.
switch c.Key {
case "http":
res.Http = c.Value
...
}
* govc package is renamed to `net` * `ProtocolProxyConfig` struct renamed to `Proxy` * vAPI accessor functions renamed to `ProxyConfig` and `NoProxyConfig`
vapi/appliance/networking/proxy.go
Outdated
| } | ||
|
|
||
| // NoProxy returns all excluded servers for proxying. | ||
| func (m *Manager) NoProxyConfig(ctx context.Context) ([]string, error) { |
There was a problem hiding this comment.
Let's leave this as you had it, NoProxy, since it matches the API name + return values.
vapi/appliance/networking/proxy.go
Outdated
| } | ||
|
|
||
| // ProxyConfig represents configuration for vcenter proxy. | ||
| type ProxyConfig struct { |
There was a problem hiding this comment.
Wondering if we should name this ProxyList to match the API
While this structure isn't a list itself, it does capture the data returned by the ProxyList API.
And same with the method below. While the structure differs from what the API returns, I don't imagine new protocols being added to the list. And if they are, we can just add to this structure.
There was a problem hiding this comment.
Agree. Renamed the struct and the method
vapi/appliance/networking/proxy.go
Outdated
| Https Proxy `json:"https,omitempty"` | ||
| } | ||
|
|
||
| // Proxy returns all Proxy configuration. |
There was a problem hiding this comment.
Also make sure the doc comment matches the type/method name.
* Ranamed `ProxyConfig` to `ProxyList` * Ranamed `NoProxyConfig` to `NoProxy`
govc: Add support for getting the VC proxy and no-proxy configuration
Introduced the vAPI bindings and the implementation for those
govc output: