Skip to content

Suggestion: use overrideCommand as false by default #3614

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

Open
felipecrs opened this issue Sep 1, 2020 · 8 comments
Open

Suggestion: use overrideCommand as false by default #3614

felipecrs opened this issue Sep 1, 2020 · 8 comments
Assignees
Labels
containers Issue in vscode-remote containers plan-review PM-highlighted item determined to be P1 or P2 under-discussion Issue is under discussion for relevance, priority, approach

Comments

@felipecrs
Copy link

felipecrs commented Sep 1, 2020

I believe there is something wrong with the concept behind this option overrideCommand, let me explain why:

In Dockerfiles you have ENTRYPOINT and CMD (command). The option overrideCommand actually overrides the ENTRYPOINT of the docker image, and not only the command (CMD). Maybe it should be renamed?

Nonetheless, I believe the entry point override should never execute by default. Entry points on Dockerfile's are scripts or programs which are supposed to run the first container initialization (they do not run in docker exec, but only on docker run) because of some customization needed. They (entry points) are responsible for correctly calling any further command passed to the docker run, or CMD if none is passed. See the official documentation here, and here.

Because of that, the official docker-from-docker devcontainer needs to customize the devcontainer.json with overrideCommand as false. See here.

If the reason for leaving it default to true is because someone can code a Dockerfile entry point in a wrong way, not set accordingly to execute the command line, so I say that this is not a VSCode fault and also not a responsibility of VSCode to work around it.

My suggestion is:

  1. Make overrideCommand overwrite CMD and not ENTRYPOINT, default is true.
  2. Create a new option overrideEntrypoint, which overrides ENTRYPOINT and not CMD, default is false.
@github-actions github-actions bot added the containers Issue in vscode-remote containers label Sep 1, 2020
@chrmarti chrmarti self-assigned this Sep 4, 2020
@chrmarti
Copy link
Contributor

chrmarti commented Sep 4, 2020

Only overriding the cmd is not enough because some images set an interpreter as the entrypoint and we want to set cmd to a process that doesn't exit. The only way to do this reliably is by overriding the entrypoint as well.

@chrmarti chrmarti added the under-discussion Issue is under discussion for relevance, priority, approach label Sep 4, 2020
@felipecrs
Copy link
Author

felipecrs commented Sep 4, 2020

@chrmarti I covered this in the description:

They (entry points) are responsible for correctly calling any further command passed to the docker run, or CMD if none is passed. See the official documentation here, and here.

Please take a look at the official docs mentioned. 1.

If the reason for leaving it default to true is because someone can code a Dockerfile entry point in a wrong way, not set accordingly to execute the command line, so I say that this is not a VSCode fault and also not a responsibility of VSCode to work around it.

As said, this can be easily addressed by documenting it. And if people want to use images with entry points bad written, they still can by setting the overrideEntrypoint to true.

@chrmarti
Copy link
Contributor

chrmarti commented Sep 4, 2020

These entrypoints are not necessarily badly written, they are just written with a different use than a DevContainer in mind.

The goal is to minimize the number of cases where the default behavior does not work and the user needs to add another flag to the devcontainer.json. I think the current default achieves that and we have the flag to turn it off to let the user take control.

@felipecrs
Copy link
Author

You hit another point which I think is crucial: how do you know that there are more users using a devcontainer which does not comply with entrypoint best practices over the ones who do? As I said, anyone who would like to use docker "from" docker would have to set this option to false, and I believe this should be a big amount of users since it's very common to have containerized applications.

And I believe we can safely say that these entrypoints are indeed badly written since they do not comply with the Docker best practices for entrypoints.

@felipecrs
Copy link
Author

@chrmarti by the way, do you know any image which would require this behavior (overwriting the entrypoint)?

@chrmarti
Copy link
Contributor

chrmarti commented Sep 7, 2020

We changed that base on the feedback in #259.

@chrmarti
Copy link
Contributor

chrmarti commented Nov 9, 2020

To avoid breaking existing configurations, we could deprecate overrideCommand and introduce overrideCmd and overrideEntrypoint. I'm not sure though if we would change the default of overrideEntrypoint to false. /cc @Chuxel

@felipecrs
Copy link
Author

felipecrs commented Nov 9, 2020

That's a nice idea. I still believe the default behavior of overrideEntrypoint should be false for the reasons stated above:

  1. It's the Docker CLI default behavior
  2. Docker recommends you to create entry points which properly execute the commands
  3. If for some reason a person does not follow this recommendation, they can still set overrideEntrypoint: true
  4. A good example of this is how Jenkins deals with Docker images as build agents. Sometimes you need entry points to properly start background services in the container. This does not mean that the entry point should not handle the CMD.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
containers Issue in vscode-remote containers plan-review PM-highlighted item determined to be P1 or P2 under-discussion Issue is under discussion for relevance, priority, approach
Projects
None yet
Development

No branches or pull requests

3 participants