Add ability to create new environments and images to the coder-sdk #100

f0ssel · 2020-08-21T20:03:27Z

These new public functions will be used for our new load testing harness for the enterprise product

cmoog

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.

coadler · 2020-08-21T21:11:54Z

coder-sdk/env.go

+	conn, resp, err := websocket.Dial(ctx, u.String(),
+		&websocket.DialOptions{
+			HTTPHeader: map[string][]string{
+				"Cookie": {"session_token=" + c.Token},


Can just use the Session-Token header instead of emulating cookies.

coadler · 2020-08-21T21:13:55Z

coder-sdk/env.go

+}
+
+// WatchEnvironmentStats watches the environment update websocket for a given duration.
+func (c Client) WatchEnvironmentStats(ctx context.Context, envID string, duration time.Duration) error {


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

True true. Should return a receive-only channel.

I think this would be much more useful if it returned some sort of EnvStatsWatcher tied to the passed ctx, so users can Read() or Close() themselves. Passing a duration seems hard to use and redundant since callers can use their own context.WithTimeout.

I tend to enjoy structs > bare channels so there's no need for selects, and so the Read function can return an error.

I think we could probably return some data-structure here that implements io.ReadCloser to piggy back on @coadler 's point.

coadler · 2020-08-21T21:15:40Z

coder-sdk/env.go

+		case <-statsCtx.Done():
+			return nil
+		default:
+			m := stats{}
+			err = wsjson.Read(ctx, conn, &m)


Can't we just pass statsCtx into wsjson.Read instead of using a select?

fuskovic

Is it cool if we hold off on merging this until Monday?

I'd like to test it against 4792 after I finish making the suggested changes and then I'll be able to provide better feedback on this.

cmoog · 2020-08-21T21:24:53Z

coder-sdk/env.go

+	statsCtx, statsCancel := context.WithTimeout(ctx, duration)
+	defer statsCancel()
+


watching cancelation should happen through the context passed above, not a new context created by the duration.

I'm not sure about this one @cmoog

I think if he passes statsCtx 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 to statsCtx but not the other way around since a time-out is not always indicative of cancellation of the parent ctx.

Oh, interesting. The two paths of cancellation is definitely a valid point. Especially considering you'd want a much shorter timeout for the initial Dial

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.

cmoog · 2020-08-21T21:26:58Z

coder-sdk/env.go

+}
+
+// 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 {


This should also return a channel that gets EnvUpdates and a higher-level wrapper would wait for "done".

cmoog · 2020-08-21T21:27:14Z

coder-sdk/env.go

+			m := stats{}
+			err = wsjson.Read(ctx, conn, &m)
+			if err != nil {
+				return fmt.Errorf("read ws json msg: %w", err)


xerrors to be consistent.

coadler · 2020-08-21T21:29:17Z

+1 for the standardization of pointer vs value returns. I lean towards returning pointers personally.

fuskovic · 2020-08-26T20:23:40Z

coder-sdk/env.go

+
+// DeleteEnvironment deletes the environment.
+func (c Client) DeleteEnvironment(ctx context.Context, envID string) error {
+	err := c.requestBody(


nitpick : Is this allocation necessary or can we just return the requestBody invocation?

fuskovic · 2020-08-26T20:39:03Z

coder-sdk/env.go

+	statsCtx, statsCancel := context.WithTimeout(ctx, duration)
+	defer statsCancel()
+


I'm not sure about this one @cmoog

I think if he passes statsCtx 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 to statsCtx but not the other way around since a time-out is not always indicative of cancellation of the parent ctx.

fuskovic · 2020-08-26T20:43:32Z

coder-sdk/env.go

+}
+
+// WatchEnvironmentStats watches the environment update websocket for a given duration.
+func (c Client) WatchEnvironmentStats(ctx context.Context, envID string, duration time.Duration) error {


I think we could probably return some data-structure here that implements io.ReadCloser to piggy back on @coadler 's point.

f0ssel · 2020-08-28T16:35:38Z

Hey @coadler , @cmoog , @fuskovic thanks for the feedback on this. I've cut down my changes to a much simpler set and will move more of the business logic into the enterprise repo to keep this cleaner for everyone while keeping the feedback here in mind.

cmoog

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.

coadler

+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.

f0ssel · 2020-08-28T18:35:46Z

@coadler thanks for the catch, just an oversight, updated to return pointers

f0ssel · 2020-08-28T18:37:40Z

@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

f0ssel force-pushed the stress-sdk branch from 74c6417 to 40e39c7 Compare August 21, 2020 20:05

f0ssel changed the title ~~Add ability to create new environments and images~~ Add ability to create new environments and images to the coder-sdk Aug 21, 2020

f0ssel requested review from cmoog and fuskovic August 21, 2020 20:32

cmoog approved these changes Aug 21, 2020

View reviewed changes

coadler suggested changes Aug 21, 2020

View reviewed changes

fuskovic reviewed Aug 21, 2020

View reviewed changes

cmoog suggested changes Aug 21, 2020

View reviewed changes

fuskovic suggested changes Aug 26, 2020

View reviewed changes

f0ssel force-pushed the stress-sdk branch 4 times, most recently from df519d9 to 902ce7b Compare August 28, 2020 16:28

f0ssel force-pushed the stress-sdk branch from 902ce7b to 792ba05 Compare August 28, 2020 16:37

f0ssel requested review from coadler, fuskovic and cmoog August 28, 2020 16:45

cmoog approved these changes Aug 28, 2020

View reviewed changes

coadler approved these changes Aug 28, 2020

View reviewed changes

Add ability to create new environments and images

af6e77c

f0ssel force-pushed the stress-sdk branch from 792ba05 to af6e77c Compare August 28, 2020 18:34

fuskovic approved these changes Aug 28, 2020

View reviewed changes

f0ssel merged commit 0f0160a into master Aug 28, 2020

f0ssel deleted the stress-sdk branch August 28, 2020 19:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to create new environments and images to the coder-sdk #100

Add ability to create new environments and images to the coder-sdk #100

f0ssel commented Aug 21, 2020

cmoog left a comment

coadler Aug 21, 2020

f0ssel Aug 28, 2020

coadler Aug 21, 2020

cmoog Aug 21, 2020

coadler Aug 21, 2020 •

edited

Loading

fuskovic Aug 26, 2020 •

edited

Loading

coadler Aug 21, 2020

fuskovic left a comment

cmoog Aug 21, 2020

fuskovic Aug 26, 2020

cmoog Aug 26, 2020

cmoog Aug 21, 2020

cmoog Aug 21, 2020

coadler commented Aug 21, 2020

fuskovic Aug 26, 2020

f0ssel Aug 28, 2020

fuskovic Aug 26, 2020

fuskovic Aug 26, 2020 •

edited

Loading

f0ssel commented Aug 28, 2020

cmoog left a comment

coadler left a comment

f0ssel commented Aug 28, 2020

f0ssel commented Aug 28, 2020

		statsCtx, statsCancel := context.WithTimeout(ctx, duration)
		defer statsCancel()

Add ability to create new environments and images to the coder-sdk #100

Add ability to create new environments and images to the coder-sdk #100

Conversation

f0ssel commented Aug 21, 2020

cmoog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coadler Aug 21, 2020 • edited Loading

Choose a reason for hiding this comment

fuskovic Aug 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fuskovic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coadler commented Aug 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fuskovic Aug 26, 2020 • edited Loading

Choose a reason for hiding this comment

f0ssel commented Aug 28, 2020

cmoog left a comment

Choose a reason for hiding this comment

coadler left a comment

Choose a reason for hiding this comment

f0ssel commented Aug 28, 2020

f0ssel commented Aug 28, 2020

coadler Aug 21, 2020 •

edited

Loading

fuskovic Aug 26, 2020 •

edited

Loading

fuskovic Aug 26, 2020 •

edited

Loading