From ed29ae99580681d6d68d792c6052949f6ff26319 Mon Sep 17 00:00:00 2001
From: Michael Sauter <mail@michaelsauter.net>
Date: Tue, 21 Nov 2017 21:12:20 +0100
Subject: [PATCH] Fix broken attach / interactive behaviour for good
---
CHANGELOG.md | 4 ++++
crane/cli.go | 6 +----
crane/container.go | 51 ++++++++++++++++++++-----------------------
crane/unit_of_work.go | 18 +++++++--------
4 files changed, 38 insertions(+), 41 deletions(-)
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 5513e95..53b87fd 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -2,6 +2,10 @@
## Unreleased
+* Remove broken flag `crane start --attach`. This flag was introduced in 3.3.0 but did not work properly, and doesn't add much value anyway. It would make more sense to have this flag on `crane run`, but that is left for another version.
+
+* Apply `--interactive` to `docker start` only if `--attach` is already passed to it.
+
## 3.3.1 (2017-11-20)
* Do not add `--interactive` to non-targeted containers, otherwise
diff --git a/crane/cli.go b/crane/cli.go
index bfeb730..44f3df3 100644
--- a/crane/cli.go
+++ b/crane/cli.go
@@ -131,10 +131,6 @@ var (
"start",
"Start the containers.",
)
- startAttachFlag = startCommand.Flag(
- "attach",
- "Attach to container",
- ).Short('a').Bool()
startTargetArg = startCommand.Arg("target", "Target of command").String()
stopCommand = app.Command(
@@ -404,7 +400,7 @@ func runCli() {
case startCommand.FullCommand():
commandAction(*startTargetArg, func(uow *UnitOfWork) {
- uow.Start(*startAttachFlag)
+ uow.Start()
}, true)
case stopCommand.FullCommand():
diff --git a/crane/container.go b/crane/container.go
index c422d05..9142136 100644
--- a/crane/container.go
+++ b/crane/container.go
@@ -22,8 +22,8 @@ type Container interface {
Provision(nocache bool)
PullImage()
Create(cmds []string)
- Run(cmds []string, detachFlag bool)
- Start(targeted bool, attachFlag bool)
+ Run(cmds []string, targeted bool, detachFlag bool)
+ Start(targeted bool)
Kill()
Stop()
Pause()
@@ -950,10 +950,8 @@ func (c *container) Create(cmds []string) {
// Implemented as create+start as we also need to connect to networks,
// and that might fail if we used "docker run" and
// have a very short-lived container.
-func (c *container) Run(cmds []string, detachFlag bool) {
+func (c *container) Run(cmds []string, targeted bool, detachFlag bool) {
adHoc := (len(cmds) > 0)
- targeted := true
- attachFlag := false
if !adHoc {
c.Rm(true, false)
@@ -966,7 +964,7 @@ func (c *container) Run(cmds []string, detachFlag bool) {
c.connectWithNetworks(adHoc)
- c.start(adHoc, targeted, attachFlag, detachFlag)
+ c.start(adHoc, targeted, detachFlag)
}
// Connects container with default network if required,
@@ -1375,17 +1373,17 @@ func (c *container) createArgs(cmds []string) []string {
}
// Start container
-func (c *container) Start(targeted bool, attachFlag bool) {
+func (c *container) Start(targeted bool) {
adHoc := false
detachFlag := false
if c.Exists() {
if !c.Running() {
c.startAcceleratedMounts()
fmt.Fprintf(c.CommandsOut(), "Starting container %s ...\n", c.ActualName(adHoc))
- c.start(adHoc, targeted, attachFlag, detachFlag)
+ c.start(adHoc, targeted, detachFlag)
}
} else {
- c.Run([]string{}, detachFlag)
+ c.Run([]string{}, targeted, detachFlag)
}
}
@@ -1399,34 +1397,33 @@ func (c *container) startAcceleratedMounts() {
}
}
-func (c *container) start(adHoc bool, targeted bool, attachFlag bool, detachFlag bool) {
+func (c *container) start(adHoc bool, targeted bool, detachFlag bool) {
executeHook(c.Hooks().PreStart(), c.ActualName(adHoc))
args := []string{"start"}
- // Attach or detach?
- // Precedence: ad-hoc > flags > config > default (targeted)
- if attachFlag || adHoc {
- args = append(args, "--attach")
- } else if !detachFlag {
- detach := !targeted
- if c.Detach != nil {
- detach = *c.Detach
- }
- if !detach {
+ // If detach is not configured, it is false by default
+ configDetach := false
+ if c.Detach != nil {
+ configDetach = *c.Detach
+ }
+
+ // It is only possible to attach to targeted containers
+ if targeted {
+ // adHoc always attaches because of --rm
+ if adHoc || (!detachFlag && !configDetach) {
args = append(args, "--attach")
+ // Interactive - implies attaching!
+ if c.Stdin_Open || c.Interactive {
+ args = append(args, "--interactive")
+ }
}
}
+
// DetachKeys
if len(c.DetachKeys()) > 0 {
args = append(args, "--detach-keys", c.DetachKeys())
}
- // Interactive
- // Implies attaching to the container, so only applicable
- // if the container is being targeted.
- if targeted && (c.Stdin_Open || c.Interactive) {
- args = append(args, "--interactive")
- }
args = append(args, c.ActualName(adHoc))
@@ -1486,7 +1483,7 @@ func (c *container) Unpause() {
func (c *container) Exec(cmds []string, privileged bool, user string) {
name := c.ActualName(false)
if !c.Running() {
- c.Start(false, false)
+ c.Start(false)
}
args := []string{"exec"}
if privileged {
diff --git a/crane/unit_of_work.go b/crane/unit_of_work.go
index 4152872..ffc57a3 100644
--- a/crane/unit_of_work.go
+++ b/crane/unit_of_work.go
@@ -77,9 +77,9 @@ func (uow *UnitOfWork) Run(cmds []string, detach bool) {
uow.prepareRequirements()
for _, container := range uow.Containers() {
if includes(uow.targeted, container.Name()) {
- container.Run(cmds, detach)
+ container.Run(cmds, true, detach)
} else if includes(uow.requireStarted, container.Name()) || !container.Exists() {
- container.Start(false, false)
+ container.Start(false)
}
}
}
@@ -89,9 +89,9 @@ func (uow *UnitOfWork) Up(cmds []string, detach bool, noCache bool, parallel int
uow.prepareRequirements()
for _, container := range uow.Containers() {
if includes(uow.targeted, container.Name()) {
- container.Run(cmds, detach)
+ container.Run(cmds, true, detach)
} else if includes(uow.requireStarted, container.Name()) || !container.Exists() {
- container.Start(false, false)
+ container.Start(false)
}
}
}
@@ -141,13 +141,13 @@ func (uow *UnitOfWork) Pause() {
}
// Start containers.
-func (uow *UnitOfWork) Start(attach bool) {
+func (uow *UnitOfWork) Start() {
uow.prepareRequirements()
for _, container := range uow.Containers() {
if includes(uow.targeted, container.Name()) {
- container.Start(true, attach)
+ container.Start(true)
} else if includes(uow.requireStarted, container.Name()) || !container.Exists() {
- container.Start(false, false)
+ container.Start(false)
}
}
}
@@ -171,7 +171,7 @@ func (uow *UnitOfWork) Exec(cmds []string, privileged bool, user string) {
if includes(uow.targeted, container.Name()) {
container.Exec(cmds, privileged, user)
} else if includes(uow.requireStarted, container.Name()) || !container.Exists() {
- container.Start(false, false)
+ container.Start(false)
}
}
}
@@ -190,7 +190,7 @@ func (uow *UnitOfWork) Create(cmds []string) {
if includes(uow.targeted, container.Name()) {
container.Create(cmds)
} else if includes(uow.requireStarted, container.Name()) || !container.Exists() {
- container.Start(false, false)
+ container.Start(false)
}
}
}