- Notifications
You must be signed in to change notification settings - Fork18
Add ability to create new environments and images to the coder-sdk#100
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looks good to me. Not that I have a strong opinion, but it'd be nice to settle on a pointer/non-pointer convention for params/return values before we ask users to use this package.
coder-sdk/env.go Outdated
| conn, resp, err := websocket.Dial(ctx, u.String(), | ||
| &websocket.DialOptions{ | ||
| HTTPHeader: map[string][]string{ | ||
| "Cookie": {"session_token=" + c.Token}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can just use theSession-Token header instead of emulating cookies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍
coder-sdk/env.go Outdated
| } | ||
| // WatchEnvironmentStats watches the environment update websocket for a given duration. | ||
| func (c Client) WatchEnvironmentStats(ctx context.Context, envID string, duration time.Duration) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
How are clients supposed to use this? It doesn't seem like anything is being done with the stats we're sent over the websocket
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
True true. Should return a receive-only channel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think this would be much more useful if it returned some sort ofEnvStatsWatcher tied to the passedctx, so users canRead() orClose() themselves. Passing a duration seems hard to use and redundant since callers can use their owncontext.WithTimeout.
I tend to enjoy structs > bare channels so there's no need for selects, and so theRead function can return an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think we could probably return some data-structure here that implementsio.ReadCloser to piggy back on@coadler 's point.
coder-sdk/env.go Outdated
| case <-statsCtx.Done(): | ||
| return nil | ||
| default: | ||
| m := stats{} | ||
| err = wsjson.Read(ctx, conn, &m) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can't we just passstatsCtx intowsjson.Read instead of using a select?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is it cool if we hold off on merging this until Monday?
I'd like to test it against4792 after I finish making the suggested changes and then I'll be able to provide better feedback on this.
coder-sdk/env.go Outdated
| statsCtx, statsCancel := context.WithTimeout(ctx, duration) | ||
| defer statsCancel() | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
watching cancelation should happen through the context passed above, not a new context created by theduration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm not sure about this one@cmoog
I think if he passesstatsCtx to the websocket read that gives us two paths to cancellation that could occur at two different times.
I think a cancellation of the parent context would propogate tostatsCtx but not the other way around since a time-out is not always indicative of cancellation of the parent ctx.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Oh, interesting. The two paths of cancellation is definitely a valid point. Especially considering you'd want a much shorter timeout for the initialDial
I'd say that makes me in favor of something similar to what Colin is suggesting where the WebSocket opener just returns some watcher structure that holds additional methods for listening that are bounded by a different context.
coder-sdk/env.go Outdated
| } | ||
| // WaitForEnvironmentReady watches the environment update websocket and waits for the "done" message type before returning. | ||
| func (c Client) WaitForEnvironmentReady(ctx context.Context, envID string) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This should also return a channel that getsEnvUpdates and a higher-level wrapper would wait for"done".
coder-sdk/env.go Outdated
| m := stats{} | ||
| err = wsjson.Read(ctx, conn, &m) | ||
| if err != nil { | ||
| return fmt.Errorf("read ws json msg: %w", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
xerrors to be consistent.
+1 for the standardization of pointer vs value returns. I lean towards returning pointers personally. |
coder-sdk/env.go Outdated
| // DeleteEnvironment deletes the environment. | ||
| func (c Client) DeleteEnvironment(ctx context.Context, envID string) error { | ||
| err := c.requestBody( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
nitpick : Is this allocation necessary or can we just return the requestBody invocation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
good call
coder-sdk/env.go Outdated
| statsCtx, statsCancel := context.WithTimeout(ctx, duration) | ||
| defer statsCancel() | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm not sure about this one@cmoog
I think if he passesstatsCtx to the websocket read that gives us two paths to cancellation that could occur at two different times.
I think a cancellation of the parent context would propogate tostatsCtx but not the other way around since a time-out is not always indicative of cancellation of the parent ctx.
coder-sdk/env.go Outdated
| } | ||
| // WatchEnvironmentStats watches the environment update websocket for a given duration. | ||
| func (c Client) WatchEnvironmentStats(ctx context.Context, envID string, duration time.Duration) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think we could probably return some data-structure here that implementsio.ReadCloser to piggy back on@coadler 's point.
df519d9 to902ce7bCompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This works for now given that we're not asking users to consume this SDK and this PR is blocking the stress-test work. That said, I'd like us to hide the underlying*websocket.Conn from our stat streaming/buildlog interactions before we expect users to depend on this package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
+1 what@cmoog said. If we intend for customers to use this, we'll need to write some wrapper around the returned ws conn.
Any specific reason for the departure from returning pointers? The usage seems a bit inconsistent now.
@coadler thanks for the catch, just an oversight, updated to return pointers |
@cmoog I agree, I can see us consuming this later similar to how the DialWsep is working currently once the stress testing stuff is in less flux |
These new public functions will be used for our new load testing harness for the enterprise product