Skip to content

Commit 423650b

Browse files
mtojekmtojek
committed
Merge branch 'main' into 3522-autogenerate-docs-2
2 parents 8c8ce44 + b103685 commit 423650b

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

41 files changed

+500
-308
lines changed

‎.github/semantic.yaml

Lines changed: 0 additions & 56 deletions
This file was deleted.

‎.github/workflows/coder.yaml

Lines changed: 34 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -338,8 +338,16 @@ jobs:
338338
else
339339
echo ::set-output name=cover::false
340340
fi
341-
set -x
342341
gotestsum --junitfile="gotests.xml" --jsonfile="gotestsum.json" --packages="./..." --debug -- -parallel=8 -timeout=3m -short -failfast $COVERAGE_FLAGS
342+
ret=$?
343+
if ((ret)); then
344+
# Eternalize test timeout logs because "re-run failed" erases
345+
# artifacts and gotestsum doesn't always capture it:
346+
# https://github.com/gotestyourself/gotestsum/issues/292
347+
echo "Checking gotestsum.json for panic trace:"
348+
grep -A 999999 'panic: test timed out' gotestsum.json
349+
fi
350+
exit $ret
343351
344352
- uses: actions/upload-artifact@v3
345353
if: success() || failure()
@@ -348,6 +356,13 @@ jobs:
348356
path: ./gotestsum.json
349357
retention-days: 7
350358

359+
- uses: actions/upload-artifact@v3
360+
if: success() || failure()
361+
with:
362+
name: gotests-${{ matrix.os }}.xml
363+
path: ./gotests.xml
364+
retention-days: 30
365+
351366
- uses: codecov/codecov-action@v3
352367
# This action has a tendency to error out unexpectedly, it has
353368
# the `fail_ci_if_error` option that defaults to `false`, but
@@ -407,7 +422,17 @@ jobs:
407422
terraform_wrapper: false
408423

409424
- name: Test with PostgreSQL Database
410-
run: make test-postgres
425+
run: |
426+
make test-postgres
427+
ret=$?
428+
if ((ret)); then
429+
# Eternalize test timeout logs because "re-run failed" erases
430+
# artifacts and gotestsum doesn't always capture it:
431+
# https://github.com/gotestyourself/gotestsum/issues/292
432+
echo "Checking gotestsum.json for panic trace:"
433+
grep -A 999999 'panic: test timed out' gotestsum.json
434+
fi
435+
exit $ret
411436
412437
- uses: actions/upload-artifact@v3
413438
if: success() || failure()
@@ -416,6 +441,13 @@ jobs:
416441
path: ./gotestsum.json
417442
retention-days: 7
418443

444+
- uses: actions/upload-artifact@v3
445+
if: success() || failure()
446+
with:
447+
name: gotests-postgres.xml
448+
path: ./gotests.xml
449+
retention-days: 30
450+
419451
- uses: codecov/codecov-action@v3
420452
# This action has a tendency to error out unexpectedly, it has
421453
# the `fail_ci_if_error` option that defaults to `false`, but

‎.github/workflows/pr.yaml

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
name: Lint PR
2+
3+
on:
4+
pull_request_target:
5+
types:
6+
- opened
7+
- reopened
8+
- edited
9+
- synchronize
10+
11+
jobs:
12+
main:
13+
name: Validate PR title
14+
runs-on: ubuntu-latest
15+
steps:
16+
- uses: amannn/action-semantic-pull-request@v5
17+
env:
18+
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
19+
with:
20+
requireScope: false

‎README.md

Lines changed: 15 additions & 13 deletions
Large diffs are not rendered by default.

‎agent/agent.go

Lines changed: 54 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ import (
3030
"github.com/spf13/afero"
3131
"go.uber.org/atomic"
3232
gossh "golang.org/x/crypto/ssh"
33+
"golang.org/x/exp/slices"
3334
"golang.org/x/xerrors"
3435
"tailscale.com/net/speedtest"
3536
"tailscale.com/tailcfg"
@@ -90,7 +91,7 @@ func New(options Options) io.Closer {
9091
}
9192
}
9293
ctx, cancelFunc := context.WithCancel(context.Background())
93-
server := &agent{
94+
a := &agent{
9495
reconnectingPTYTimeout: options.ReconnectingPTYTimeout,
9596
logger: options.Logger,
9697
closeCancel: cancelFunc,
@@ -101,8 +102,8 @@ func New(options Options) io.Closer {
101102
filesystem: options.Filesystem,
102103
tempDir: options.TempDir,
103104
}
104-
server.init(ctx)
105-
return server
105+
a.init(ctx)
106+
return a
106107
}
107108

108109
type agent struct {
@@ -300,10 +301,12 @@ func (a *agent) createTailnet(ctx context.Context, derpMap *tailcfg.DERPMap) (_
300301
}
301302
}()
302303
if err = a.trackConnGoroutine(func() {
304+
logger := a.logger.Named("reconnecting-pty")
305+
303306
for {
304307
conn, err := reconnectingPTYListener.Accept()
305308
if err != nil {
306-
a.logger.Debug(ctx, "accept pty failed", slog.Error(err))
309+
logger.Debug(ctx, "accept pty failed", slog.Error(err))
307310
return
308311
}
309312
// This cannot use a JSON decoder, since that can
@@ -324,7 +327,9 @@ func (a *agent) createTailnet(ctx context.Context, derpMap *tailcfg.DERPMap) (_
324327
if err != nil {
325328
continue
326329
}
327-
go a.handleReconnectingPTY(ctx, msg, conn)
330+
go func() {
331+
_ = a.handleReconnectingPTY(ctx, logger, msg, conn)
332+
}()
328333
}
329334
}); err != nil {
330335
return nil, err
@@ -798,38 +803,56 @@ func (a *agent) handleSSHSession(session ssh.Session) (retErr error) {
798803
return cmd.Wait()
799804
}
800805

801-
func (a *agent) handleReconnectingPTY(ctx context.Context, msg codersdk.ReconnectingPTYInit, conn net.Conn) {
806+
func (a *agent) handleReconnectingPTY(ctx context.Context, logger slog.Logger, msg codersdk.ReconnectingPTYInit, conn net.Conn) (retErr error) {
802807
defer conn.Close()
803808

804809
connectionID := uuid.NewString()
810+
logger = logger.With(slog.F("id", msg.ID), slog.F("connection_id", connectionID))
811+
812+
defer func() {
813+
if err := retErr; err != nil {
814+
a.closeMutex.Lock()
815+
closed := a.isClosed()
816+
a.closeMutex.Unlock()
817+
818+
// If the agent is closed, we don't want to
819+
// log this as an error since it's expected.
820+
if closed {
821+
logger.Debug(ctx, "session error after agent close", slog.Error(err))
822+
} else {
823+
logger.Error(ctx, "session error", slog.Error(err))
824+
}
825+
}
826+
logger.Debug(ctx, "session closed")
827+
}()
828+
805829
var rpty *reconnectingPTY
806830
rawRPTY, ok := a.reconnectingPTYs.Load(msg.ID)
807831
if ok {
832+
logger.Debug(ctx, "connecting to existing session")
808833
rpty, ok = rawRPTY.(*reconnectingPTY)
809834
if !ok {
810-
a.logger.Error(ctx, "found invalid type in reconnecting pty map", slog.F("id", msg.ID))
811-
return
835+
return xerrors.Errorf("found invalid type in reconnecting pty map: %T", rawRPTY)
812836
}
813837
} else {
838+
logger.Debug(ctx, "creating new session")
839+
814840
// Empty command will default to the users shell!
815841
cmd, err := a.createCommand(ctx, msg.Command, nil)
816842
if err != nil {
817-
a.logger.Error(ctx, "create reconnecting pty command", slog.Error(err))
818-
return
843+
return xerrors.Errorf("create command: %w", err)
819844
}
820845
cmd.Env = append(cmd.Env, "TERM=xterm-256color")
821846

822847
// Default to buffer 64KiB.
823848
circularBuffer, err := circbuf.NewBuffer(64 << 10)
824849
if err != nil {
825-
a.logger.Error(ctx, "create circular buffer", slog.Error(err))
826-
return
850+
return xerrors.Errorf("create circular buffer: %w", err)
827851
}
828852

829853
ptty, process, err := pty.Start(cmd)
830854
if err != nil {
831-
a.logger.Error(ctx, "start reconnecting pty command", slog.F("id", msg.ID), slog.Error(err))
832-
return
855+
return xerrors.Errorf("start command: %w", err)
833856
}
834857

835858
ctx, cancelFunc := context.WithCancel(ctx)
@@ -873,7 +896,7 @@ func (a *agent) handleReconnectingPTY(ctx context.Context, msg codersdk.Reconnec
873896
_, err = rpty.circularBuffer.Write(part)
874897
rpty.circularBufferMutex.Unlock()
875898
if err != nil {
876-
a.logger.Error(ctx, "reconnecting pty write buffer", slog.Error(err), slog.F("id", msg.ID))
899+
logger.Error(ctx, "write to circular buffer", slog.Error(err))
877900
break
878901
}
879902
rpty.activeConnsMutex.Lock()
@@ -889,23 +912,27 @@ func (a *agent) handleReconnectingPTY(ctx context.Context, msg codersdk.Reconnec
889912
rpty.Close()
890913
a.reconnectingPTYs.Delete(msg.ID)
891914
}); err != nil {
892-
a.logger.Error(ctx, "start reconnecting pty routine", slog.F("id", msg.ID), slog.Error(err))
893-
return
915+
return xerrors.Errorf("start routine: %w", err)
894916
}
895917
}
896918
// Resize the PTY to initial height + width.
897919
err := rpty.ptty.Resize(msg.Height, msg.Width)
898920
if err != nil {
899921
// We can continue after this, it's not fatal!
900-
a.logger.Error(ctx, "resize reconnecting pty", slog.F("id", msg.ID), slog.Error(err))
922+
logger.Error(ctx, "resize", slog.Error(err))
901923
}
902924
// Write any previously stored data for the TTY.
903925
rpty.circularBufferMutex.RLock()
904-
_, err = conn.Write(rpty.circularBuffer.Bytes())
926+
prevBuf := slices.Clone(rpty.circularBuffer.Bytes())
905927
rpty.circularBufferMutex.RUnlock()
928+
// Note that there is a small race here between writing buffered
929+
// data and storing conn in activeConns. This is likely a very minor
930+
// edge case, but we should look into ways to avoid it. Holding
931+
// activeConnsMutex would be one option, but holding this mutex
932+
// while also holding circularBufferMutex seems dangerous.
933+
_, err = conn.Write(prevBuf)
906934
if err != nil {
907-
a.logger.Warn(ctx, "write reconnecting pty buffer", slog.F("id", msg.ID), slog.Error(err))
908-
return
935+
return xerrors.Errorf("write buffer to conn: %w", err)
909936
}
910937
// Multiple connections to the same TTY are permitted.
911938
// This could easily be used for terminal sharing, but
@@ -946,16 +973,16 @@ func (a *agent) handleReconnectingPTY(ctx context.Context, msg codersdk.Reconnec
946973
for {
947974
err = decoder.Decode(&req)
948975
if xerrors.Is(err, io.EOF) {
949-
return
976+
return nil
950977
}
951978
if err != nil {
952-
a.logger.Warn(ctx, "reconnecting pty buffer read error", slog.F("id", msg.ID), slog.Error(err))
953-
return
979+
logger.Warn(ctx, "read conn", slog.Error(err))
980+
return nil
954981
}
955982
_, err = rpty.ptty.Input().Write([]byte(req.Data))
956983
if err != nil {
957-
a.logger.Warn(ctx, "write to reconnecting pty", slog.F("id", msg.ID), slog.Error(err))
958-
return
984+
logger.Warn(ctx, "write to pty", slog.Error(err))
985+
return nil
959986
}
960987
// Check if a resize needs to happen!
961988
if req.Height == 0 || req.Width == 0 {
@@ -964,7 +991,7 @@ func (a *agent) handleReconnectingPTY(ctx context.Context, msg codersdk.Reconnec
964991
err = rpty.ptty.Resize(req.Height, req.Width)
965992
if err != nil {
966993
// We can continue after this, it's not fatal!
967-
a.logger.Error(ctx, "resize reconnecting pty", slog.F("id", msg.ID), slog.Error(err))
994+
logger.Error(ctx, "resize", slog.Error(err))
968995
}
969996
}
970997
}

‎cli/create_test.go

Lines changed: 29 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -232,13 +232,16 @@ func TestCreate(t *testing.T) {
232232
ProvisionApply: echo.ProvisionComplete,
233233
ProvisionPlan: echo.ProvisionComplete,
234234
})
235+
235236
coderdtest.AwaitTemplateVersionJob(t, client, version.ID)
236-
template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID)
237+
_ = coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID)
238+
237239
tempDir := t.TempDir()
238240
removeTmpDirUntilSuccessAfterTest(t, tempDir)
239241
parameterFile, _ := os.CreateTemp(tempDir, "testParameterFile*.yaml")
240-
_, _ = parameterFile.WriteString("zone: \"bananas\"")
241-
cmd, root := clitest.New(t, "create", "my-workspace", "--template", template.Name, "--parameter-file", parameterFile.Name())
242+
_, _ = parameterFile.WriteString("username: \"boingo\"")
243+
244+
cmd, root := clitest.New(t, "create", "", "--parameter-file", parameterFile.Name())
242245
clitest.SetupConfig(t, client, root)
243246
doneChan := make(chan struct{})
244247
pty := ptytest.New(t)
@@ -247,11 +250,32 @@ func TestCreate(t *testing.T) {
247250
go func() {
248251
defer close(doneChan)
249252
err := cmd.Execute()
250-
assert.EqualError(t, err, "Parameter value absent in parameter file for \"region\"!")
253+
assert.NoError(t, err)
251254
}()
255+
matches := []struct {
256+
match string
257+
write string
258+
}{
259+
{
260+
match: "Specify a name",
261+
write: "my-workspace",
262+
},
263+
{
264+
match: fmt.Sprintf("Enter a value (default: %q):", defaultValue),
265+
write: "bingo",
266+
},
267+
{
268+
match: "Confirm create?",
269+
write: "yes",
270+
},
271+
}
272+
273+
for _, m := range matches {
274+
pty.ExpectMatch(m.match)
275+
pty.WriteLine(m.write)
276+
}
252277
<-doneChan
253278
})
254-
255279
t.Run("FailedDryRun", func(t *testing.T) {
256280
t.Parallel()
257281
client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true})

0 commit comments

Comments
 (0)