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

Add logic to specify exec commands during postStart and preStop #272

Merged
merged 1 commit into from
Sep 13, 2015
Merged

Add logic to specify exec commands during postStart and preStop #272

merged 1 commit into from
Sep 13, 2015

Conversation

DennisDenuto
Copy link
Contributor

PR for #266
e.g.

<wait>
  <http>
    <url>http://localhost:${host.port}</url>
    <method>GET</method>
    <status>200..399</status>
  </http>
  <time>10000</time>
  <shutdown>500</shutdown>
  <exec>
    <postStart>/opt/init_db.sh<postStart>
    <preStop>/opt/notify_end.sh</preStop>
  </exec>
</wait>

@rhuss
Copy link
Collaborator

rhuss commented Aug 31, 2015

Thanks for the PR, I just did a quick review. Looks real good overall. Some minor remarks:

  • Could you please add some documentation to manual.md (with example) and to changelog.md ?
  • /wr to the possible options when attaching to a running container: I never would expect that a tty would be set for a maven build, so I would remove Tty all together. Also I always would expect to attach to StdErr and StdOut and currently its also not possible to set it from the configuration. So I would remove that, too, and then maybe the whole ExecContainerConfiguration since all what is left is the command. We could make it a org.jolokia.docker.maven.config.Arguments as it is for the other shell-command related config options, too (though I don't know by heart whether there are also two forms (exec/shell) for calling the exec container.
  • I would remove Optional in ShutdownAction because this can be realized easily with a null-check, too and wouldn't introduce a new dependency.

A general principle I try to follow is to implement only what is needed (and don't fall in the trap we might need it sometime as I did to often and frankly nearly never happened) and even when I think things like Optional are a good thing, I want to introduce that only when there is a good reason weighting out the costs of an extra dependency. From my feeling this is not yet the case yet.

Signed-off-by: Dennis Leon <dennis.leon39@gmail.com>
@DennisDenuto
Copy link
Contributor Author

Thanks for the feedback!
From what I can see the /exec endpoint only accepts 'exec' not 'shell'. (Not sure if that changes your opinion on whether Arguments should still be used since it maintains both shell and exec.)

wrt Optional, I figured it was ok to use since guava was on the classpath and was only a private helper method. But I agree looking back now it looks out of place considering null checks are mainly used. :-)

@DennisDenuto
Copy link
Contributor Author

Hey Rhuss,
were you able to check the changes made to this PR? (following your feedback).

@rhuss
Copy link
Collaborator

rhuss commented Sep 13, 2015

Ooops, didn't get any update by GitHub that the PR was changed. Will have a look ASAP.

@rhuss rhuss merged commit 8d33a6e into fabric8io:integration Sep 13, 2015
rhuss added a commit that referenced this pull request Sep 13, 2015
Removed some bogus imports, add property docs + minor cleanup related to #272, #266
@rhuss
Copy link
Collaborator

rhuss commented Sep 13, 2015

Thanks again ! I'm going to cleanup the duplicate code in ShutdownAction and RunService, too, since there is now some overlap here.

@DennisDenuto DennisDenuto deleted the add-wait-exec branch September 13, 2015 17:36
@DennisDenuto
Copy link
Contributor Author

Cool Thanks heaps Rhuss

@robzet
Copy link

robzet commented Jun 30, 2016

Can I use this to run more than one command? I cant seam to get it working. I have tried with two commands separated by ; or && (as &&). Only one command is ran.I can see both commands getting sent over rest interface as a "Cmd": "[]" but I get errors related to it and it does not look like its interpreted as two commands correctly. Any ideas?

As i believe the first answer will be "put the commands in a script and execute that in one command" I want to also add some background to the problem. My setup runs well on Linux, but not on Windows. The command is supposed to run a script file with several commands in. On Windows this file looses its permissions somehow during transfer to the docker container and this is why I need to run two commands. One to restore +x and one to execute the script.

@rhuss
Copy link
Collaborator

rhuss commented Jun 30, 2016

There is a known issue with Windows which looses the execution permission. For a complete discussion please refer to #477, but in short with 0.15.9 you have the following options:

  • You use an assembly with an assembly descriptor which adds your postExec script. In this assembly descriptor you can add a file mode for the entries in a fileSet. Then you create the assembly with a <mode>zip</mode> (or tar, but not <mode>dir</mode> since you will loose the permission again).
  • You add a <permissions>exec</permissions> to the assembly descriptor which will add the exec bit to every file. A somewhat ugly workaround but the way that Docker recommends.

Does this help ?

As a workaround you could also use 'sh /path/to/my/script' as command to explicitly call your script, too, so you dont have to rely on the execution bit.

@robzet
Copy link

robzet commented Jun 30, 2016

@rhuss Thank you very much! That worked well. I can now run the script because the execute permissions are kept on windows as well after changing the assembly mode to 'tar' in the pom-file.

@burtsevyg
Copy link

My configuration

                                        <wait>
                                            <time>30000</time>
                                            <exec>
                                                <postStart>/opt/postStart.sh</postStart>
                                            </exec>
                                        </wait>

/opt/postStart.sh return 1 exit code.
Why postStart script did not fail maven build?
How can I do this.

@rhuss
Copy link
Collaborator

rhuss commented Sep 1, 2016

Currently the exit status of a postStart is not checked at all (would require an extra API call). That's probably not that difficult, but its simply not implemented yet.

If you think this would be a useful addition, then please open an extra issue for that, describing your use case. thanks !

leusonmario pushed a commit to leusonmario/docker-maven-plugin that referenced this pull request Aug 18, 2018
…rmediate_images_for_circleci

Add ability to send --rm param on build (seems needed for CircleCI)
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.

4 participants