cmd: Add enter command #188

woky · 2023-01-31T15:08:50Z

The enter command behaves just like run when run without positional
arguments, with the exception that the --hold flag is not supported.
That is, it runs as service manager. When run with positional arguments,
the service manager is started in background without starting autostart
services (i.e. like run with the --hold flag) and positional arguments
are executed as pebble command. Only subset of pebble commands is
supported, namely exec, help, ls, plan, services, start, stop and
version. Autostart services can be started with the --run flag. start
command must be run with the --run flag and ls, plan, services and stop
commands cannot be run with the --run flag. Normally, the service
manager is shut down when the command finishes, except for start and
stop commands where the service manager continues running. The enter
command is intended to be used as entrypoint in container images.

woky · 2023-01-31T15:12:27Z

@niemeyer @benhoyt Note that I've included my https://github.com/woky/go-flags/ fork which supports setting PassAfterNonOption to subset of commands. I've a opened a PR against upstream: jessevdk/go-flags#393 But given the activity of the maintainer on the GitHub, I suspect it won't get any attention anytime soon. There's also other PR to go-flags incoming from @rebornplusplus adding support to find -exec-like terminated option arguments. Should we consider forking go-flags?

benhoyt

I haven't reviewed this in detail (let me know if it's ready for that and if you want that), but just left a couple of preliminary comments.

cmd/pebble/cmd_enter.go

benhoyt · 2023-02-01T17:00:15Z

cmd/pebble/cmd_enter.go

+	var runPanic interface{}
+
+	go func() {
+		defer func() { runPanicCh <- recover() }()


Why don't we just let a panic crash the process?

This is because in cmd/ we actually use panic({exitStatus(1)}) as kind of long jump. When exitStatus error is recovered in main, the main just exits with the given status code. The run command uses it for restart functionality and also to bail when something goes wrong. If I let the goroutine panic, the exitStatus error wouldn't be recovered in main. And, since exitStatus has Error method, if run command wanted to exit with status 1, then the following would be printed

panic: internal error: exitStatus{1} being handled as normal error goroutine 6 [running]: main.(*cmdRun).run(0x0?, 0x0?) /home/woky/devel/pebble/cmd/pebble/cmd_run.go:82 +0xa9 main.(*cmdEnter).Execute.func2() /home/woky/devel/pebble/cmd/pebble/cmd_enter.go:140 +0x25 created by main.(*cmdEnter).Execute /home/woky/devel/pebble/cmd/pebble/cmd_enter.go:138 +0x46a exit status 2

instead of just

exit status 1

cmd/pebble/main.go

woky · 2023-02-05T23:58:32Z

@benhoyt I've addressed your comments. It won't be ready to merge unless either the PR to go-flags is merged or we decide to fork it (and how), but I think it's ready to review.

niemeyer

That's surprisingly tidy, thanks Tomas.

Just a few minors and some high-level questions:

niemeyer · 2023-02-06T10:14:12Z

cmd/pebble/cmd_enter.go

+without starting "autostart" services (i.e. run --hold, unless --run flag is
+given), and the subcommand is run as a pebble client command that can
+communicate with the service manager instance in the background. The service
+manager is shut down after the commands finishes unless noted otherwise below.


This is doing a reasonable job of describing the behavior, but as Ben already spotted it is also very confusing because the actual behavior of any one command is hard to grasp because it's an ANDing of a number of different conditional paragraphs that may affect the given command or not. The agreement we've reached at the end of our several meetings was supposed to be one where the behavior of enter would be the most natural option for the given command at hand, so if we did that justice, we shoud be able to have a more straightforward description here too.

With that said, let's leave that aside for now and focus on the implementation. Once this is ready to go in, we can do a pass together and find the best way to word this out.

I agree it's not exactly straightforward read.

cmd/pebble/cmd_enter.go

niemeyer · 2023-02-06T10:41:34Z

cmd/pebble/cmd_enter.go

+	}
+
+	if enterFlags&enterFlagRequireServiceAutostart != 0 && !cmd.Run {
+		return fmt.Errorf("enter: this command can only run with autostarted services (hint: add --run to enter)")


Perhaps more directly:

enter: must use --run before %q subcommand

With that said, the memory I have of our final agreement (which arguably took a bit of back and forth, so I may easily be missing something) was that we'd make subcommands behave in their most natural way, and when there was no natural way at all we'd prevent the subcommand from running.

In particular here, the fact we'd have start and stop disagreeing on the requirement of --run, and the fact we're forcing --run to be provided to a subcommand that could not work without it, are both bad symptoms that this isn't quite what we need. Let's talk about it elsewhere to revisit this topic please.

I wholeheartedly agree that the sub-command behavior as currently specified is confusing and hard to convey.

EDIT: Changed the messages according to your example.

cmd/pebble/cmd_enter.go

niemeyer · 2023-02-06T10:48:37Z

cmd/pebble/cmd_enter.go

+	err := commander.Execute(extraArgs)
+
+	if err != nil || enterFlags&enterFlagKeepServiceManager == 0 {
+		runStop()


Haven't reached the end of the code yet, so maybe this will be answered later, but how are we keeping the run command going if we're returning from enter early here? In other words, the fact we're not asking the run command to stop explicitly won't do much for us if this command is about to return and exit from Pebble itself.

This is handled by runResultCh (previously called runPanicCh) that's written to in the goroutine closure above after the run command ends, and that's being read from right after this statement.

benhoyt

This looks good to me. Can we add some simple tests for a few of the pebble enter use cases? Might be more trouble than it's worth, but worth some thought IMO.

benhoyt · 2023-02-20T01:04:20Z

cmd/pebble/cmd_enter.go

+Furthermore, some commands have restrictions and idiosyncrasies:
+
+  exec, ls, plan, services, version
+    Service manager logging is disabled unless -v/--verbose flag is provided.


The wording still needs a few style/grammar tweaks I think. But for now, just note that these indentations are removed when displaying the help (i.e., running pebble enter -h).

Noted, thanks. This is done by wrapText function in go-flags. Let's keep it indented and fix go-flags later.

cmd/pebble/cmd_enter.go

woky · 2023-03-02T02:38:14Z

@niemeyer @benhoyt I've addressed most of your comments (and replied to the rest). Please take a look again.

Note the CI failures. This because of data race detector detecting race condition when setting logger with logger.SetLogger():

Data race detector output

==================
WARNING: DATA RACE
Write at 0x00000117add0 by goroutine 336:
  github.com/canonical/pebble/internal/logger.SetLogger()
      /home/woky/canonical/devel/pebble/internal/logger/logger.go:83 +0x15e
  github.com/canonical/pebble/cmd/pebble.run()
      /home/woky/canonical/devel/pebble/cmd/pebble/main.go:343 +0x36
  github.com/canonical/pebble/cmd/pebble.main()
      /home/woky/canonical/devel/pebble/cmd/pebble/main.go:324 +0x58
  github.com/canonical/pebble/cmd/pebble.RealMain()
      /home/woky/canonical/devel/pebble/cmd/pebble/export_test.go:77 +0xdc
  github.com/canonical/pebble/cmd/pebble_test.(*PebbleSuite).TestEnterHelpCommand()
      /home/woky/canonical/devel/pebble/cmd/pebble/cmd_enter_test.go:79 +0x22c
  runtime.call16()
      /usr/lib/go/src/runtime/asm_amd64.s:728 +0x48
  reflect.Value.Call()
      /usr/lib/go/src/reflect/value.go:370 +0xc7
  gopkg.in/check%2ev1.(*suiteRunner).forkTest.func1()
      /home/woky/go/pkg/mod/gopkg.in/[email protected]/check.go:775 +0xadd
  gopkg.in/check%2ev1.(*suiteRunner).forkCall.func1()
      /home/woky/go/pkg/mod/gopkg.in/[email protected]/check.go:669 +0xec

Previous read at 0x00000117add0 by goroutine 319:
  github.com/canonical/pebble/internal/logger.Debugf()
      /home/woky/canonical/devel/pebble/internal/logger/logger.go:66 +0x6e
  github.com/canonical/pebble/internal/overlord/cmdstate.(*execution).controlLoop()
      /home/woky/canonical/devel/pebble/internal/overlord/cmdstate/handlers.go:464 +0x10e4
  github.com/canonical/pebble/internal/overlord/cmdstate.(*execution).do.func9()
      /home/woky/canonical/devel/pebble/internal/overlord/cmdstate/handlers.go:272 +0x84

Goroutine 336 (running) created at:
  gopkg.in/check%2ev1.(*suiteRunner).forkCall()
      /home/woky/go/pkg/mod/gopkg.in/[email protected]/check.go:666 +0x5c4
  gopkg.in/check%2ev1.(*suiteRunner).forkTest()
      /home/woky/go/pkg/mod/gopkg.in/[email protected]/check.go:757 +0x16b
  gopkg.in/check%2ev1.(*suiteRunner).runTest()
      /home/woky/go/pkg/mod/gopkg.in/[email protected]/check.go:812 +0x439
  gopkg.in/check%2ev1.(*suiteRunner).run()
      /home/woky/go/pkg/mod/gopkg.in/[email protected]/check.go:618 +0x3e6
  gopkg.in/check%2ev1.Run()
      /home/woky/go/pkg/mod/gopkg.in/[email protected]/run.go:92 +0x49
  gopkg.in/check%2ev1.RunAll()
      /home/woky/go/pkg/mod/gopkg.in/[email protected]/run.go:84 +0x12b
  gopkg.in/check%2ev1.TestingT()
      /home/woky/go/pkg/mod/gopkg.in/[email protected]/run.go:72 +0x565
  github.com/canonical/pebble/cmd/pebble_test.Test()
      /home/woky/canonical/devel/pebble/cmd/pebble/main_test.go:24 +0x2e
  testing.tRunner()
      /usr/lib/go/src/testing/testing.go:1576 +0x216
  testing.(*T).Run.func1()
      /usr/lib/go/src/testing/testing.go:1629 +0x47

Goroutine 319 (finished) created at:
  github.com/canonical/pebble/internal/overlord/cmdstate.(*execution).do()
      /home/woky/canonical/devel/pebble/internal/overlord/cmdstate/handlers.go:272 +0xfc5
  github.com/canonical/pebble/internal/overlord/cmdstate.(*CommandManager).doExec()
      /home/woky/canonical/devel/pebble/internal/overlord/cmdstate/handlers.go:118 +0xb09
  github.com/canonical/pebble/internal/overlord/cmdstate.(*CommandManager).doExec-fm()
      <autogenerated>:1 +0x4d
  github.com/canonical/pebble/internal/overlord/state.(*TaskRunner).run.func1()
      /home/woky/canonical/devel/pebble/internal/overlord/state/taskrunner.go:195 +0xb3
  gopkg.in/tomb%2ev2.(*Tomb).run()
      /home/woky/go/pkg/mod/gopkg.in/[email protected]/tomb.go:163 +0x51
  gopkg.in/tomb%2ev2.(*Tomb).Go.func2()
      /home/woky/go/pkg/mod/gopkg.in/[email protected]/tomb.go:159 +0x47
==================

I've never debugged data races in Golang so I could use some help. However, my theory is that this is a non-issue and it only surfaced because this is the first test where pebble is started as a service manager (overlord) and so logger functions are called from goroutines other than the one where pebble/cmd/pebble_test.Test() is called. The logger.SetLogger() call is made only in main() and only once. In actual pebble enter execution the main() is called exactly once (in the same process). The sequence of the steps in data race detector report (write after previous read) indicates that this is an issue with access between individual tests (i.e. tests are not fully isolated between each other).

I could disable the the data race detector in the new test file with // +build !race but that doesn't seem right. Synchronizing access to the logger also doesn't seem right as that'd make logging expensive. What are your thoughts?

woky · 2023-03-02T15:28:38Z

I've fixed the data race detector warning by synchronizing access to the global logger variable. After discussion with @niemeyer we concluded it's fine.

The currently failing test in Go 1.15 is unrelated:

2023-03-02T15:20:33.4744199Z START: daemon_test.go:727: daemonSuite.TestRebootHelper
2023-03-02T15:20:33.4744628Z START: daemon_test.go:64: daemonSuite.SetUpTest
2023-03-02T15:20:33.4744979Z PASS: daemon_test.go:64: daemonSuite.SetUpTest	0.000s
2023-03-02T15:20:33.4745184Z 
2023-03-02T15:20:33.4745346Z using shellcheck: "/usr/bin/shellcheck"
2023-03-02T15:20:33.4745658Z daemon_test.go:744:
2023-03-02T15:20:33.4745942Z     c.Assert(err, check.IsNil)
2023-03-02T15:20:33.4746406Z ... value *os.SyscallError = &os.SyscallError{Syscall:"waitid", Err:0xa} ("waitid: no child processes")
2023-03-02T15:20:33.4746683Z 
2023-03-02T15:20:33.4746866Z START: daemon_test.go:73: daemonSuite.TearDownTest
2023-03-02T15:20:33.4747262Z PASS: daemon_test.go:73: daemonSuite.TearDownTest	0.000s
2023-03-02T15:20:33.4747479Z 
2023-03-02T15:20:33.4747635Z FAIL: daemon_test.go:727: daemonSuite.TestRebootHelper

EDIT: It didn't happen again (after I force pushed with updated commit date).

benhoyt · 2023-03-02T23:48:50Z

Looks good to me (apart from perhaps the help wording which it sounds like you and Gustavo will work on). And thanks for the logging fix!

niemeyer

Here is the proposed help text, and we forgot to talk about stop vs. --run yesterday.

Otherwise, this looks good to go.

niemeyer · 2023-03-03T16:55:41Z

cmd/pebble/cmd_enter.go

+)
+
+const shortEnterHelp = "Run pebble command in new pebble environment"
+const longEnterHelp = `


Okay, here is the proposal

Short help: Run subcommand under a container environment

Long help:

The enter command facilitates the use of Pebble as an entrypoint for containers. When used without a subcommand it mimics the behavior of the run command alone, while if used with a subcommand it runs that subcommand in the most appropriate environment taking in account its purpose. These subcommands are currently supported: help (1)(2) version (1)(2) plan (1)(2) services (1)(2) ls (1)(2) exec (2) start (3) stop (3) (1) Services are not started. (2) No logs on stdout unless -v is used. (3) Services continue running after the subcommand succeeds.

looks good.

nitpick:

< taking in account > taking into account

niemeyer · 2023-03-03T16:57:05Z

cmd/pebble/cmd_enter.go

+    Service manager continues running after the command finishes.
+
+  stop
+    Service autostart (the --run flag) is required.


We forgot to address this wart in our call yesterday. We need to figure why start/stop were proposed to be different, as it's very awkward to require a flag at all times for stop and not for start.

Recalling the spec (RK007) discussions, the logic was that pebble enter stop [svc ...] would have an equivalent behaviour to pebble run --hold; pebble stop [svc ...], which was deemed to be undesired and thus marked as an "error" combination.

Service autostart (the --run flag) is required.

Putting this in other words, --run is required as a consequence of pebble enter acting as pebble run --hold. Without it, enter stop doesn't do anything cause there are no "started" services.

niemeyer

Alright, thanks for all the reviews and help here.

This is good to go, pending only the go-flags support.

Update: Sorry, forgot to mention the one trivial below:

cmd/pebble/cmd_enter.go

niemeyer · 2023-03-12T14:20:58Z

Given the lack of response from upstream, let's temporarily fork go-flags at https://github.com/canonical/go-flags, and submit/review the required extensions there please.

woky · 2023-03-13T03:01:36Z

Given the lack of response from upstream, let's temporarily fork go-flags at https://github.com/canonical/go-flags, and submit/review the required extensions there please.

Thanks. Opened PR canonical/go-flags#1

woky · 2023-03-20T18:33:55Z

@niemeyer @benhoyt There was a bug in subcommand --help and help command handling. I've fixed it in last commit added to this PR: 0eb575e

woky · 2023-03-30T10:26:23Z

Updated go.{mod|sum} for canonical/go-flags fork. Tests pass! Should be good to go. :o)

niemeyer

Thanks, Tomas. One trivial only before we merge:

cmd/pebble/cmd_enter.go

niemeyer · 2023-04-03T16:51:37Z

One more trivial, sorry about that: formatting checks are failing.

Also, run go mod tidy.

This obviates the need to pass double dash (--) when exec command includes command line switches.

In main.go we define Stdin, Stdout and Stderr reader/writers. This is to aid with testing. In cmd_exec.go we use os.Std* files as reader/writers. This makes it impossible to test output from exec commands. Change cmd_exec.go to use reader/writers defined in main.go.

Currently run() from main.go is exported to tests as RunMain, but we cannot test pebble exits with specific error code with run(). Make main() exit via osExit function variable and export RealMain that catches and returns the exit code.

From the long help message: Usage: pebble enter [enter-OPTIONS] [<subcommand>...] The enter command facilitates the use of Pebble as an entrypoint for containers. When used without a subcommand it mimics the behavior of the run command alone, while if used with a subcommand it runs that subcommand in the most appropriate environment taking into account its purpose. These subcommands are currently supported: help (1)(2) version (1)(2) plan (1)(2) services (1)(2) ls (1)(2) exec (2) start (3) stop (3) (1) Services are not started. (2) No logs on stdout unless -v is used. (3) Services continue running after the subcommand succeeds. [enter command options] --create-dirs Create pebble directory on startup if it doesn't exist --hold Do not start default services automatically --http= Start HTTP API listening on this address (e.g., ":4000") -v, --verbose Log all output from services to stdout --run Start default services before executing subcommand

In practice logger.SetDefault() is only called once in main() and never again. This sets up global logger variable which is then read by logger print functions (Panicf, Noticef, Debugf) in other parts of pebble. However, test runs are not fully isolated and multiple tests can call logger.SetDefaul() in the same process. Until now, this wasn't a problem, because no test called logger.SetDefault() and logger print functions in different goroutines. But when we start pebble as a service manager inside a test, like in cmd/pebble/cmd_enter_test.go, several goroutines are created, some of which call logger print functions. In this case it's likely that logger.SetDefault() gets called in another test after it was read from different goroutine in a logger print function call. When that happens, the golang data race detetor, which is active during test runs in CI, reports the problem included at the bottom. Since logger.SetLogger() is public it can in theory be called by other code. Synchronize access to it by newly introduced mutex. Also drop mutex from defaultLogger structure as it's private data type and access to it inside logger packages is already protected by the newly introduced mutex. [1] https://go.dev/doc/articles/race_detector Golang data race detector report: ================== WARNING: DATA RACE Write at 0x00000117add0 by goroutine 336: github.com/canonical/pebble/internal/logger.SetLogger() /home/woky/devel/devel/pebble/internal/logger/logger.go:83 +0x15e github.com/canonical/pebble/cmd/pebble.run() /home/woky/devel/devel/pebble/cmd/pebble/main.go:343 +0x36 github.com/canonical/pebble/cmd/pebble.main() /home/woky/devel/devel/pebble/cmd/pebble/main.go:324 +0x58 github.com/canonical/pebble/cmd/pebble.RealMain() /home/woky/devel/devel/pebble/cmd/pebble/export_test.go:77 +0xdc github.com/canonical/pebble/cmd/pebble_test.(*PebbleSuite).TestEnterHelpCommand() /home/woky/devel/devel/pebble/cmd/pebble/cmd_enter_test.go:79 +0x22c runtime.call16() /usr/lib/go/src/runtime/asm_amd64.s:728 +0x48 reflect.Value.Call() /usr/lib/go/src/reflect/value.go:370 +0xc7 gopkg.in/check%2ev1.(*suiteRunner).forkTest.func1() /home/woky/go/pkg/mod/gopkg.in/[email protected]/check.go:775 +0xadd gopkg.in/check%2ev1.(*suiteRunner).forkCall.func1() /home/woky/go/pkg/mod/gopkg.in/[email protected]/check.go:669 +0xec Previous read at 0x00000117add0 by goroutine 319: github.com/canonical/pebble/internal/logger.Debugf() /home/woky/devel/devel/pebble/internal/logger/logger.go:66 +0x6e github.com/canonical/pebble/internal/overlord/cmdstate.(*execution).controlLoop() /home/woky/devel/devel/pebble/internal/overlord/cmdstate/handlers.go:464 +0x10e4 github.com/canonical/pebble/internal/overlord/cmdstate.(*execution).do.func9() /home/woky/devel/devel/pebble/internal/overlord/cmdstate/handlers.go:272 +0x84 Goroutine 336 (running) created at: gopkg.in/check%2ev1.(*suiteRunner).forkCall() /home/woky/go/pkg/mod/gopkg.in/[email protected]/check.go:666 +0x5c4 gopkg.in/check%2ev1.(*suiteRunner).forkTest() /home/woky/go/pkg/mod/gopkg.in/[email protected]/check.go:757 +0x16b gopkg.in/check%2ev1.(*suiteRunner).runTest() /home/woky/go/pkg/mod/gopkg.in/[email protected]/check.go:812 +0x439 gopkg.in/check%2ev1.(*suiteRunner).run() /home/woky/go/pkg/mod/gopkg.in/[email protected]/check.go:618 +0x3e6 gopkg.in/check%2ev1.Run() /home/woky/go/pkg/mod/gopkg.in/[email protected]/run.go:92 +0x49 gopkg.in/check%2ev1.RunAll() /home/woky/go/pkg/mod/gopkg.in/[email protected]/run.go:84 +0x12b gopkg.in/check%2ev1.TestingT() /home/woky/go/pkg/mod/gopkg.in/[email protected]/run.go:72 +0x565 github.com/canonical/pebble/cmd/pebble_test.Test() /home/woky/devel/devel/pebble/cmd/pebble/main_test.go:24 +0x2e testing.tRunner() /usr/lib/go/src/testing/testing.go:1576 +0x216 testing.(*T).Run.func1() /usr/lib/go/src/testing/testing.go:1629 +0x47 Goroutine 319 (finished) created at: github.com/canonical/pebble/internal/overlord/cmdstate.(*execution).do() /home/woky/devel/devel/pebble/internal/overlord/cmdstate/handlers.go:272 +0xfc5 github.com/canonical/pebble/internal/overlord/cmdstate.(*CommandManager).doExec() /home/woky/devel/devel/pebble/internal/overlord/cmdstate/handlers.go:118 +0xb09 github.com/canonical/pebble/internal/overlord/cmdstate.(*CommandManager).doExec-fm() <autogenerated>:1 +0x4d github.com/canonical/pebble/internal/overlord/state.(*TaskRunner).run.func1() /home/woky/devel/devel/pebble/internal/overlord/state/taskrunner.go:195 +0xb3 gopkg.in/tomb%2ev2.(*Tomb).run() /home/woky/go/pkg/mod/gopkg.in/[email protected]/tomb.go:163 +0x51 gopkg.in/tomb%2ev2.(*Tomb).Go.func2() /home/woky/go/pkg/mod/gopkg.in/[email protected]/tomb.go:159 +0x47 ==================

This commit fixes handling of the following two cases: 1. pebble enter subcommand --help 2. pebble enter help subcommand Help messages are handled in main.go when the executed command returns flags.Error{Type:flags.ErrHelp}. When we pass the subcommand's return value back to the main parser, the error handling there will use the currently active command to print the help message. The active command is enter but we want to print the subcommands help message. Whenever the subcommand returns an error value, set it as active command in the main parser, so that the error handling there works with the failing subcommand.

woky · 2023-04-03T17:18:25Z

One more trivial, sorry about that: formatting checks are failing.

Whoops. Fixed. (And squashed into first commit.)

woky marked this pull request as draft January 31, 2023 15:09

woky force-pushed the pebble-enter-20230119 branch from 635cedb to 42bae10 Compare January 31, 2023 21:49

benhoyt reviewed Feb 1, 2023

View reviewed changes

woky force-pushed the pebble-enter-20230119 branch from 1d1fff6 to ba90c45 Compare February 5, 2023 23:30

woky marked this pull request as ready for review February 6, 2023 09:08

woky force-pushed the pebble-enter-20230119 branch 2 times, most recently from 903f427 to 8ce91b0 Compare February 6, 2023 09:14

niemeyer requested changes Feb 6, 2023

View reviewed changes

woky requested review from benhoyt and niemeyer and removed request for benhoyt February 6, 2023 11:45

rebornplusplus mentioned this pull request Feb 8, 2023

feat: Provide arguments to service via pebble run --args #191

Merged

benhoyt approved these changes Feb 20, 2023

View reviewed changes

woky force-pushed the pebble-enter-20230119 branch 2 times, most recently from 9367d14 to 299c308 Compare March 2, 2023 01:39

This was referenced Mar 2, 2023

Add support for the Pebble entrypoint canonical/rockcraft#205

Merged

Test race conditions (including cases only seen under heavy cpu load) #181

Closed

woky force-pushed the pebble-enter-20230119 branch from 8a06b17 to 2b49c9d Compare March 2, 2023 15:19

woky force-pushed the pebble-enter-20230119 branch 2 times, most recently from 907ceb7 to 2300ba6 Compare March 2, 2023 21:13

benhoyt approved these changes Mar 2, 2023

View reviewed changes

niemeyer requested changes Mar 3, 2023

View reviewed changes

woky requested a review from niemeyer March 8, 2023 09:06

woky force-pushed the pebble-enter-20230119 branch 2 times, most recently from 03f1289 to 3253d55 Compare March 8, 2023 09:09

niemeyer approved these changes Mar 12, 2023

View reviewed changes

cmd/pebble/cmd_enter.go Outdated Show resolved Hide resolved

woky force-pushed the pebble-enter-20230119 branch from 3253d55 to 6b1d386 Compare March 13, 2023 02:57

cjdcordeiro approved these changes Mar 20, 2023

View reviewed changes

woky force-pushed the pebble-enter-20230119 branch from 470e6ff to d331c3c Compare March 20, 2023 18:31

woky force-pushed the pebble-enter-20230119 branch from d331c3c to 0eb575e Compare March 20, 2023 18:36

cjdcordeiro added the High Priority Look at me first label Mar 22, 2023

woky force-pushed the pebble-enter-20230119 branch from 0eb575e to d870575 Compare March 30, 2023 10:11

niemeyer requested changes Mar 31, 2023

View reviewed changes

cmd/pebble/cmd_enter.go Outdated Show resolved Hide resolved

woky force-pushed the pebble-enter-20230119 branch 2 times, most recently from 8b953d1 to 82ce86c Compare April 3, 2023 11:31

woky requested a review from niemeyer April 3, 2023 11:32

woky force-pushed the pebble-enter-20230119 branch 2 times, most recently from b95b567 to dba41c0 Compare April 3, 2023 11:37

woky added 7 commits April 3, 2023 19:15

Use canonical/go-flags fork

fe49cd0

Also, run go mod tidy.

cmd/exec: Set PassAfterNonOption for exec

2cdb52b

This obviates the need to pass double dash (--) when exec command includes command line switches.

cmd: Export main() to tests as RealMain

01179dd

Currently run() from main.go is exported to tests as RunMain, but we cannot test pebble exits with specific error code with run(). Make main() exit via osExit function variable and export RealMain that catches and returns the exit code.

woky force-pushed the pebble-enter-20230119 branch from dba41c0 to a346470 Compare April 3, 2023 17:16

niemeyer merged commit 0c8bb88 into canonical:master Apr 6, 2023

rebornplusplus mentioned this pull request Jul 11, 2023

enter: start default services before executing subcommand #257

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmd: Add enter command #188

cmd: Add enter command #188

woky commented Jan 31, 2023 •

edited

Loading

woky commented Jan 31, 2023

benhoyt left a comment

benhoyt Feb 1, 2023

woky Feb 5, 2023

woky commented Feb 5, 2023

niemeyer left a comment

niemeyer Feb 6, 2023

woky Feb 6, 2023

niemeyer Feb 6, 2023

woky Feb 6, 2023 •

edited

Loading

niemeyer Feb 6, 2023

woky Feb 6, 2023

benhoyt left a comment

benhoyt Feb 20, 2023

woky Mar 2, 2023

woky commented Mar 2, 2023 •

edited

Loading

woky commented Mar 2, 2023 •

edited

Loading

benhoyt commented Mar 2, 2023

niemeyer left a comment

niemeyer Mar 3, 2023 •

edited

Loading

cjdcordeiro Mar 3, 2023

niemeyer Mar 3, 2023

cjdcordeiro Mar 3, 2023 •

edited

Loading

niemeyer left a comment •

edited

Loading

niemeyer commented Mar 12, 2023 •

edited

Loading

woky commented Mar 13, 2023

woky commented Mar 20, 2023 •

edited

Loading

woky commented Mar 30, 2023

niemeyer left a comment

niemeyer commented Apr 3, 2023

woky commented Apr 3, 2023

cmd: Add enter command #188

cmd: Add enter command #188

Conversation

woky commented Jan 31, 2023 • edited Loading

woky commented Jan 31, 2023

benhoyt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woky commented Feb 5, 2023

niemeyer 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

woky Feb 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benhoyt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woky commented Mar 2, 2023 • edited Loading

woky commented Mar 2, 2023 • edited Loading

benhoyt commented Mar 2, 2023

niemeyer left a comment

Choose a reason for hiding this comment

niemeyer Mar 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjdcordeiro Mar 3, 2023 • edited Loading

Choose a reason for hiding this comment

niemeyer left a comment • edited Loading

Choose a reason for hiding this comment

niemeyer commented Mar 12, 2023 • edited Loading

woky commented Mar 13, 2023

woky commented Mar 20, 2023 • edited Loading

woky commented Mar 30, 2023

niemeyer left a comment

Choose a reason for hiding this comment

niemeyer commented Apr 3, 2023

woky commented Apr 3, 2023

woky commented Jan 31, 2023 •

edited

Loading

woky Feb 6, 2023 •

edited

Loading

woky commented Mar 2, 2023 •

edited

Loading

woky commented Mar 2, 2023 •

edited

Loading

niemeyer Mar 3, 2023 •

edited

Loading

cjdcordeiro Mar 3, 2023 •

edited

Loading

niemeyer left a comment •

edited

Loading

niemeyer commented Mar 12, 2023 •

edited

Loading

woky commented Mar 20, 2023 •

edited

Loading