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

docker-py expect only `command` argument not `cmd` #27444

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
3 participants
@ticosax
Copy link
Contributor

commented Sep 27, 2015

docker.Client.create_container expect command and not cmd
#27331

@terminalmage

This comment has been minimized.

Copy link
Contributor

commented Sep 27, 2015

This probably isn't the correct fix, please don't merge this yet until I get a chance to look closer.

@isbm

This comment has been minimized.

Copy link
Contributor

commented Sep 28, 2015

@terminalmage Also it would be better to have a deprecation notification. I.e. still have command as an argument for backward compatibility, but let it complain that in the next version it will be removed. An example is here, although in your case it is only an argument name.

@ticosax

This comment has been minimized.

Copy link
Contributor Author

commented Sep 28, 2015

@isbm command belongs to docker namespace. I'd like to keep it in the context of docker because it feels more natural than cmd. But keeping both doesn't really help neither.

So my question, why this renaming in the first place ?

@isbm

This comment has been minimized.

Copy link
Contributor

commented Sep 28, 2015

@ticosax Because:

  1. It is not natural to write SLS and in the same file have cmd and command, where only dockerng and postfix looks like aliens and all the rest of Salt modules use cmd.
  2. Salt context is a king here, because it encapsulates everything. That means there should not be brought to the common interface of Salt also exec, execute, run, call etc as well, just only because some of these feels it more natural to that module context.
  3. There is no problem to alias parameters. In reality, docker is often used with other things, even just direct calling of something. It looks messy when one formula does the same operations at steps, but each step looks different.

I'd also add that there is no such thing as "natural to the module", but "consistent with the Salt". For example, there are many things that are natural to a Zypper, a bit natural to the Yum, but absolutely unnatural to the Dpkg. But at the end it should be all the same. Same applies to the different filesystems, where the same operations should be called the same way, e.g. you "mount", not "connect" or "attach" or "publish" etc.

@ticosax

This comment has been minimized.

Copy link
Contributor Author

commented Sep 28, 2015

@isbm Thank you for the detailed explanations.

I understand salt encapsulation is strong over utilities that are gathered under the same umbrella. But for standalone api like docker, when salt Interface shadows docker api I think it brings more complexity. Because I never been able to achieve my goals without understand docker api. At the end, I feel more comfortable with docker api. In this scenario salt become "just" a yaml interface where I forgot all the things it solves for me.
This opinion is based on my experience. It is not something I consider right or wrong.

So, thank you again for your answer, I understand better, and long live to cmd !

@terminalmage

This comment has been minimized.

Copy link
Contributor

commented Sep 28, 2015

OK, after looking at this, the fix from @ticosax is good after all. I misread it yesterday when I was in the middle of working on something else.

The issue here is that I did things backwards. I should have made kwargs['cmd'] be popped and placed into kwargs['command'], not the other way around. docker-py still expects command to be used.

There were some differences in the docstrings and comments that I wanted to make, so I opened a new PR (#27459) for this. I then incorporated the additional test from @ticosax into that PR, with a slightly modified commit message.

There is no deprecation to be done, both arguments will be accepted and equivalent, as explained in the docstrings for the dockerng.running state and dockerng.create remote execution function.

@terminalmage

This comment has been minimized.

Copy link
Contributor

commented Sep 28, 2015

I'm going to close this as #27459 incorporates this entire PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.