Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow docker command to be null, set default CMD in dockerfile instead #1615

Merged
merged 2 commits into from
Dec 12, 2018

Conversation

fabianvf
Copy link
Contributor

@fabianvf fabianvf commented Dec 5, 2018

Fixes #1614

PR Type

  • Bugfix Pull Request

Instead of setting the default command value of while true; do sleep 10000; done in the docker_container invocation, create the sample Dockerfile with the default CMD on it, and allow command to be nullable, in which case it will use the existing CMD directive on the image.

@fabianvf fabianvf force-pushed the molecule-docker-cmd branch from 07bee14 to 6351a34 Compare December 5, 2018 22:09
@gundalow gundalow added this to the v2.20 milestone Dec 6, 2018
@gundalow gundalow added the bug label Dec 6, 2018
@fabianvf
Copy link
Contributor Author

fabianvf commented Dec 6, 2018

Need to update the tests that are relying on the docket default command directive, will try to get that pushed this morning.

@gundalow gundalow changed the title Allow docker command to be null, set default CMD in dockerfile instead [WIP] Allow docker command to be null, set default CMD in dockerfile instead Dec 6, 2018
@fabianvf fabianvf force-pushed the molecule-docker-cmd branch from 6351a34 to 7f4a0b1 Compare December 6, 2018 22:33
@fabianvf
Copy link
Contributor Author

fabianvf commented Dec 7, 2018

I can't reproduce these failures locally, and it looks like master is failing with the same issue. It looks like it's not finding the molecule binary or something.

@gundalow @chris-short

@fabianvf fabianvf force-pushed the molecule-docker-cmd branch from 7f4a0b1 to de8eba3 Compare December 10, 2018 16:10
@fabianvf fabianvf changed the title [WIP] Allow docker command to be null, set default CMD in dockerfile instead Allow docker command to be null, set default CMD in dockerfile instead Dec 10, 2018
@fabianvf
Copy link
Contributor Author

I'm unable to reproduce the test failure in travis, and as far as I can tell there's no useful debug output in the travis log, can someone rekick the build to verify it's not a flake? @gundalow @webknjaz

@fabianvf fabianvf force-pushed the molecule-docker-cmd branch from de8eba3 to 75f9064 Compare December 10, 2018 22:03
@fabianvf fabianvf force-pushed the molecule-docker-cmd branch from 039019a to d97d43d Compare December 11, 2018 21:01
@webknjaz
Copy link
Member

@fabianvf you can restart tests by just reopening PR if needed.

Those previous failure was related to #1611 (combo of tox-dev/tox#1097 + psf/requests#4890), which is fixed now.

@fabianvf
Copy link
Contributor Author

@webknjaz I think I actually found the issue, there was a separate Dockerfile template for tests that didn't have the CMD directive in it. Think this should work now.

@webknjaz
Copy link
Member

Let's wait for results now. I've canceled the previous build because it was blocking the queue in CI.

@webknjaz

This comment has been minimized.

@webknjaz webknjaz merged commit 40a08a6 into ansible:master Dec 12, 2018
decentral1se added a commit to decentral1se/molecule that referenced this pull request Feb 22, 2019
Closes ansible#1763.

This adds a `override_command` key to the platforms definition. It
defaults to true. A user who wants to override the `CMD` directive from
their `Dockerfile.j2` can specify `override_command: False` to have it
honoured.

This allows 3 use cases to be covered:

  * Getting the zero configuration default of `bash -c ...`
  * Overriding the `CMD` directive from the `command` key
  * Overriding the `CMD` directive from the `Dockerfile.j2` and setting `override_command: false`

See also:

  * ansible#1615
  * ansible#1614
  * ansible#1441

Signed-off-by: Luke Murphy <[email protected]>
decentral1se added a commit to decentral1se/molecule that referenced this pull request Feb 24, 2019
Closes ansible#1763.

This adds a `override_command` key to the platforms definition. It
defaults to true. A user who wants to override the `CMD` directive from
their `Dockerfile.j2` can specify `override_command: False` to have it
honoured.

This allows 3 use cases to be covered:

  * Getting the zero configuration default of `bash -c ...`
  * Overriding the `CMD` directive from the `command` key
  * Overriding the `CMD` directive from the `Dockerfile.j2` and setting `override_command: false`

See also:

  * ansible#1615
  * ansible#1614
  * ansible#1441

Signed-off-by: Luke Murphy <[email protected]>
decentral1se added a commit that referenced this pull request Feb 25, 2019
Closes #1763.

This adds a `override_command` key to the platforms definition. It
defaults to true. A user who wants to override the `CMD` directive from
their `Dockerfile.j2` can specify `override_command: False` to have it
honoured.

This allows 3 use cases to be covered:

  * Getting the zero configuration default of `bash -c ...`
  * Overriding the `CMD` directive from the `command` key
  * Overriding the `CMD` directive from the `Dockerfile.j2` and setting `override_command: false`

See also:

  * #1615
  * #1614
  * #1441

Signed-off-by: Luke Murphy <[email protected]>
ssbarnea pushed a commit to ssbarnea/molecule that referenced this pull request Feb 27, 2019
Closes ansible#1763.

This adds a `override_command` key to the platforms definition. It
defaults to true. A user who wants to override the `CMD` directive from
their `Dockerfile.j2` can specify `override_command: False` to have it
honoured.

This allows 3 use cases to be covered:

  * Getting the zero configuration default of `bash -c ...`
  * Overriding the `CMD` directive from the `command` key
  * Overriding the `CMD` directive from the `Dockerfile.j2` and setting `override_command: false`

See also:

  * ansible#1615
  * ansible#1614
  * ansible#1441

Signed-off-by: Luke Murphy <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CMD in pre-built docker image replaced with while true; do sleep 10000; done
3 participants