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

support for additional container configuration options #60

Closed
wants to merge 1 commit into from

Conversation

jgangemi
Copy link
Collaborator

in addition fixing #55, the following options can be set on the run configuration:

  • hostname
  • domainname
  • user
  • memory
  • memorySwap
  • entrypoint
  • workingDir
  • privileged
  • dns
  • dnsSearch
  • capAdd
  • capDrop
  • restartPolicy

- allow container configuration parameters to be specified for
   - hostname
   - domainname
   - user
   - memory
   - memorySwap
   - entrypoint
   - volumes
   - workingDir
   - binds
   - privileged
   - dns
   - dnsSearch
   - capAdd
   - capDrop
   - restartPolicy

Signed-off-by: Jae Gangemi <jgangemi@gmail.com>
@rhuss
Copy link
Collaborator

rhuss commented Dec 2, 2014

Thanks a lot !! Frankly, I never heard of most of those new run options yet ;-)

From a support and documentation point of view I wonder whether we should offer all possibilities of the Docker API and intentionally restrict to a (reasonable) subset. My feelings are a bit ambivalent here. Of course it's harder to support a bigger feature set with features which are not really needed within the scope of this plugin, but on the other hand it is probably equally hard to define a "reasonable" subset (since usage patterns might vary, e.g. I use it for integration tests only).

But I tend to your suggestion to add all those options, especially since it can make integrations tests more realistic because one could add the same parameters as on production (especially thinks like 'memory'). And if mimicking the Docker API also helps in an easier adoption (instead of eg. inventing new semantics).

Some other remarks:

  • We probably will need a documentation, too. Could you add some to manual.md (maybe with refering to the official Docker documentation for some feature where appilcable).
  • I wouldn't set the API version hard to 1.15 since this will break the plugin for usage with older docker installations even if they don't use "newer" features. Maybe we could select the API version based on the feature used (e.g. when building up the ContainerHostConfig one could keep track the highest required API version based on the setters used and the use this API) ? That way we can support older docker installations if newer features are not used ...
  • Could we rename ContainerConfig --> ContainerCreateConfig and ContainerHostConfig --> ContainerStartConfig (if I understand them right) ?

@rhuss
Copy link
Collaborator

rhuss commented Dec 2, 2014

[I made the last comment already yesterday but forgot to posit. Mee, getting old ;-)

@jgangemi
Copy link
Collaborator Author

jgangemi commented Dec 2, 2014

i included almost every option i felt would be relevant. there are still some that i left out however if someone wanted them, they would be very easy to add after once these changes are merged.

i can update the docs and rename the classes.

as for the api version - given docker had a bunch of security holes that were just recently fixed, ppl should be running the latest version but i realize that may not be realistic for all. tracking what version to use based on what setters are called would be cool but is it really worth all that extra work? what if we offered an override for the api version via the pom?

two other things...

  1. could you take a look at configuring run ports via external properties is limiting #61 and offer some input/feedback? i'd like to finish that off.

  2. how is log functionality coming? i ask b/c i started to refactor the client to provide the error messages that come out of docker but then saw some comment you made related to that issue, looked at the code and saw you had a bunch of changes and decided to wait to avoid a merge hell.

also b/c i'm still doing mvn docker:logs ;)

@rhuss
Copy link
Collaborator

rhuss commented Dec 2, 2014

Thanks for the feedback.

I'll think about the API stuff, would be cool if the plugin is smart enough to pick the highest needed version (not only for now but also for the evolution of Docker in general). But you are right, maybe its not worth the effort (although I don't think that there are many options which 'bump' up the API version).

To your question

  1. I will comment on configuring run ports via external properties is limiting #61 ASAP.
  2. Logging comes along quite nicely and is nearly finished. You can try it out on branch 8-logs (for a quick start use -Ddocker.showLogs, but there are many more options and a dedicated target, too). I'll add some documentation and merge it over to master soon.

@rhuss
Copy link
Collaborator

rhuss commented Dec 3, 2014

I just pushed a snapshot 0.10.5-SNAPSHOT to maven central with the new logging stuff, maybe you can try this out ?

Documentation for this can be found here: https://github.com/rhuss/docker-maven-plugin/blob/8-logs/doc/manual.md

@jgangemi
Copy link
Collaborator Author

jgangemi commented Dec 3, 2014

do you think it'd be safe to pull those changes into the branch i have for this? i'm using a locally built copy of the plugin right now b/c i need this functionality for something at the day job.

@rhuss
Copy link
Collaborator

rhuss commented Dec 3, 2014

I think its save, but you can try this by first creating a new branch from your current branch and try a rebase on that first (you can remove this branch later on).

Is your branch 'issue55' up to date ? If so, I would pull it into the main repository as an extra branch and do the rebase, so that you can use this one for your work (and fork that for your further work on this PR).

@rhuss
Copy link
Collaborator

rhuss commented Dec 3, 2014

I pulled your branch 'issue55', rebased it with the log stuff and pushed it to my branch at https://github.com/rhuss/docker-maven-plugin/tree/55-export-volumes

There where some merge conflicts, which could be resolved.

When you work on this PR, could you please fork this 55-export-volumes branch and work on this ? (and send further PRs from there) ?

@jgangemi
Copy link
Collaborator Author

jgangemi commented Dec 3, 2014

yeah - that sounds good, thanks!

@rhuss
Copy link
Collaborator

rhuss commented Dec 5, 2014

I'm going to make a release today (0.10.5) with the logging stuff, changes from this pull request will the go into 0.10.6. Hope that's ok for you ....

Release fast, release often .... ;-)

@rhuss
Copy link
Collaborator

rhuss commented Dec 5, 2014

If you don't mind I again rebase the 55-export-volumes with the just released 0.10.5 (assuming you are not already did some local changes)

@jgangemi
Copy link
Collaborator Author

jgangemi commented Dec 5, 2014

meh - i was just working on this last night. to try and finish it up - time zone differences...

i'll pull and get this done today.

@rhuss
Copy link
Collaborator

rhuss commented Dec 5, 2014

No problem. You can do a pull request without rebasing, shouldn't be a problem since there are only minor changes.

@jgangemi
Copy link
Collaborator Author

jgangemi commented Dec 5, 2014

did you push that rebase? i'm trying to pull from the branch and git tells me we are in sync but that can't be the case if you renamed log to logs

@jgangemi
Copy link
Collaborator Author

jgangemi commented Dec 5, 2014

i guess can just pull from master

@rhuss
Copy link
Collaborator

rhuss commented Dec 5, 2014

No, I didn't rebased the latest changes from 'master' to '55-export-volumes' but can do so. 'wanted to ask before rebasing for not messing up your changes you did in the meantime.

@rhuss
Copy link
Collaborator

rhuss commented Dec 5, 2014

Yes, pulling from master should be fine (hope so ;-)

@jgangemi
Copy link
Collaborator Author

jgangemi commented Dec 5, 2014

cool - i pulled from master so we're all good.

@jgangemi
Copy link
Collaborator Author

jgangemi commented Dec 6, 2014

replaced by #65

@jgangemi jgangemi closed this Dec 6, 2014
leusonmario pushed a commit to leusonmario/docker-maven-plugin that referenced this pull request Aug 18, 2018
Support auth credentials for private repo push
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