Skip to content

docker-compose exec doesn't have -e option (fixes #4551)#5237

Merged
shin- merged 1 commit intodocker:masterfrom
garribas:4551-exec-does-not-have-env-option
Oct 25, 2017
Merged

docker-compose exec doesn't have -e option (fixes #4551)#5237
shin- merged 1 commit intodocker:masterfrom
garribas:4551-exec-does-not-have-env-option

Conversation

@garribas
Copy link

@garribas garribas commented Oct 5, 2017

Signed-off-by: Guillermo Arribas garribas@gmail.com

Had to refactor a little bit method TopLevelCommand.exec_command to reduce cyclomatic complexity and avoid pre commit hooks error.

@garribas garribas force-pushed the 4551-exec-does-not-have-env-option branch from c901db6 to 20e9bb9 Compare October 14, 2017 01:52
@shin- shin- added this to the 1.18.0 milestone Oct 19, 2017
Copy link

@shin- shin- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of small modifications, looks good overall.

}

# setting environment for exec is not supported in API < 1.25'
if docker.utils.version_gte(self.project.client.api_version, '1.25'):
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should call out that limitation in the docstring above, and raise a UserError if the API version is too low.

])

# env overridden
self.assertIn('foo=notbar', stdout)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we can use assert 'foo=notbar' in stdout instead (and similarly for the lines below), which looks a bit nicer.

@garribas garribas force-pushed the 4551-exec-does-not-have-env-option branch from 20e9bb9 to acc55b6 Compare October 20, 2017 01:03
Signed-off-by: Guillermo Arribas <garribas@gmail.com>
@garribas garribas force-pushed the 4551-exec-does-not-have-env-option branch from acc55b6 to 968cd11 Compare October 20, 2017 01:07
@garribas
Copy link
Author

Thanks for the feedback, made requested changes.

@shin- shin- merged commit b30cb77 into docker:master Oct 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants