Conversation
abhimanyubabbar
force-pushed
the
private/babbara/enable-proxy-setting
branch
from
19e197a to
128346a
Compare
…ing hostagent through proxy
abhimanyubabbar
force-pushed
the
private/babbara/enable-proxy-setting
branch
from
64eca32 to
af7d420
Compare
roopakparikh
left a comment
roopakparikh
left a comment
There was a problem hiding this comment.
Some minor comments, else looks good, we do have overlapping changes, but since your code preceded mine, please go ahead and merge after making the suggested changes.
| func New(fqdn string, proxy string) (Client, error) { | ||
|
|
||
| http, err := NewHTTP( | ||
| func(impl *HTTPImpl) { impl.Proxy = proxy }, |
There was a problem hiding this comment.
Why are we passing in functions ? Why don't we just pass in the value ?
| Qbert Qbert | ||
| Executor Executor | ||
| Segment Segment | ||
| HTTP HTTP |
There was a problem hiding this comment.
I would call it an HTTPClient
| Keystone: NewKeystone(fqdn, http), | ||
| Qbert: NewQbert(fqdn, http), | ||
| Executor: ExecutorImpl{}, | ||
| Segment: NewSegment(fqdn), |
There was a problem hiding this comment.
I would like to understand more on how we have used in different places, in general I would like to see more "events" and segment should be able to "consume" those events instead of directly using segment.
|
|
||
| var transport http.RoundTripper | ||
| if resp.Proxy != "" { | ||
| proxyURL, err := url.Parse(resp.Proxy) |
There was a problem hiding this comment.
We should parse it when user enters the proxy and error out. ProxyURL can be used inside the code instead of passing proxy as string
There was a problem hiding this comment.
Fail at the input site only you mean ?!
| t := rehttp.NewTransport(transport, rehttp.RetryAll( | ||
| rehttp.RetryAny( | ||
| rehttp.RetryTemporaryErr(), | ||
| rehttp.RetryStatuses(400, 404), |
There was a problem hiding this comment.
What about when gateway times out, I think it is 502 or 503 I can't recall
There was a problem hiding this comment.
RetryTemporaryErr() covers them I think. I'll check
| resp, err := util.AskBool("Prep local node for kubernetes cluster") | ||
| if err != nil || !resp { | ||
| log.Errorf("Couldn't fetch user content") | ||
| prep, err := util.AskBool("PrepLocal node for kubernetes cluster") |
There was a problem hiding this comment.
we should protect these via a silent option, in other words if there is "--silent" present we should move forward without asking.
| if err := PrepNode(ctx, c, "", "", "", []string{}); err != nil { | ||
| return fmt.Errorf("Unable to prepnode: %w", err) | ||
| if prep { | ||
| err = PrepNode(ctx, c, "", "", "", []string{}) |
There was a problem hiding this comment.
put a comment to talk about the parameters
|
|
||
| // Host interface exposes the functions | ||
| // required to setup the host correctly. | ||
| type Host interface { |
|
|
||
| // SwapOff disables the swap. | ||
| func (h Debian) SwapOff() error { | ||
| return h.exec.Run("bash", "-c", "swapoff -a") |
There was a problem hiding this comment.
This is not persistent
| @@ -21,11 +21,12 @@ type Keystone interface { | |||
| } | |||
|
|
|||
| type KeystoneImpl struct { | |||
There was a problem hiding this comment.
In terms of the organization, I would actually move up all of these: Keystone.go, Resmgr.go into their own packages: /pkg/keystone/keystone.go; /pkg/resmgr/resmgr.go;...
2 participants
No description provided.