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

Create shared start/stop scripts for better extensibility #367

Merged
merged 9 commits into from
Nov 28, 2018

Conversation

glorpen
Copy link
Contributor

@glorpen glorpen commented Oct 30, 2018

This PR replaces current docker-runscript with scripts for service start and stop actions - used by systems with and without systemd.
There is not much added functionality for end-user, just singular new parameter - after_create in docker::run. In my current use case it will allow to easily add connecting container to multiple networks (since docker create is now always used).
Additionally this PR opens a way to add handling for named user/group from host system, eg. --user $(id -u user):$(id -g group). (PR's for both will come later)

Currently the first one is possible by overusing extra_systemd_parameters parameter and second one is not even possible because of systemd $() handling.

Main changes:

  • no ExecStartPre/ExecStop(Post) in systemd service
  • shared logic for start/stop actions (shell script), based on existing upstart code
  • docker create is now always used in init scripts, no docker run

@davejrt
Copy link
Contributor

davejrt commented Nov 27, 2018

@glorpen can you please resolve the merge conflict here and we can look at getting this merged for the next release.

Copy link
Contributor

@davejrt davejrt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please remove all detach params and documentation pertaining to this param, as it's being removed.

@glorpen
Copy link
Contributor Author

glorpen commented Nov 28, 2018

@davejrt Done, I hope I've catched all run::detach uses.

@davejrt
Copy link
Contributor

davejrt commented Nov 28, 2018

LGTM

@davejrt davejrt merged commit fc67e28 into puppetlabs:master Nov 28, 2018
@davejrt
Copy link
Contributor

davejrt commented Dec 16, 2018

@glorpen looks like this is breaking on systems using upstart. Primarily the -a flag with docker start. Do you want to push a fix, or we'll have to revert the PR so it doesn't break our next release.

@glorpen
Copy link
Contributor Author

glorpen commented Dec 16, 2018

@davejrt hmmm, wouldn't reverting f5de8ac fix it? I didn't noticed it before but $valid_detach is still used in inline script and it should result in absence of -a flag when not on systemd and now it is undef so I guess always systemd behavior.

@davejrt
Copy link
Contributor

davejrt commented Dec 16, 2018

I think you're correct, can you revert it on your fork and push that as the commits are squashed on the merge of the PR

@glorpen
Copy link
Contributor Author

glorpen commented Dec 17, 2018

Done

@davejrt
Copy link
Contributor

davejrt commented Dec 18, 2018

Do you want to submit the fix as a PR?

@glorpen
Copy link
Contributor Author

glorpen commented Dec 19, 2018

If it is required, I'm fine with you just merging/squashing/etc it.

@glorpen
Copy link
Contributor Author

glorpen commented Jan 2, 2019

@davejrt not sure if repo silence is caused by holidays or not; let me know if you want me to create that PR after all :)

@davejrt
Copy link
Contributor

davejrt commented Jan 2, 2019

Happy Holidays, if you could create the PR that would be great

@davejrt davejrt added this to the 3.2.0 milestone Jan 10, 2019
@Ramesh7 Ramesh7 added the bugfix label Jul 5, 2023
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.

3 participants