Skip to content

Generate Systemd #2985

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.

Sign up for GitHub

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

Jump to bottom
Merged
openshift-merge-robot merged 1 commit into from
May 2, 2019
Merged

Generate Systemd #2985

openshift-merge-robot merged 1 commit into from
May 2, 2019

Conversation

@baude
Copy link
Member

@baude baude commented Apr 22, 2019

Signed-off-by: baude bbaude@redhat.com

@openshift-ci-robot openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L labels Apr 22, 2019
@openshift-ci-robot
Copy link
Collaborator

openshift-ci-robot commented Apr 22, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: baude

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 22, 2019
EmilienM
EmilienM reviewed Apr 22, 2019
View reviewed changes
pkg/systemdgen/systemdgen.go Outdated
Copy link
Contributor

@EmilienM EmilienM Apr 22, 2019

Choose a reason for hiding this comment

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

I've tested this config and I confirm it is what works the best for us in OpenStack.

EmilienM
EmilienM reviewed Apr 22, 2019
View reviewed changes
EmilienM
EmilienM reviewed Apr 22, 2019
View reviewed changes
Copy link
Contributor

@EmilienM EmilienM left a comment

Choose a reason for hiding this comment

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

Except the minor comment on the "stop" command, it looks good to me. Thanks for this work!

rhatdan
rhatdan reviewed Apr 22, 2019
View reviewed changes
@mheon
Copy link
Member

mheon commented Apr 22, 2019

@baude Can you hit the podman run manpage, remove the default unit file in the examples, and recommend they use this instead?

mheon
mheon reviewed Apr 22, 2019
View reviewed changes
@baude
Copy link
Member Author

baude commented Apr 22, 2019

I just put this PR up so I could get feedback from @EmilienM ... still lots of things to do.

adrianreber
adrianreber reviewed Apr 23, 2019
View reviewed changes
@baude baude force-pushed the generatesystemd branch from 9ceb7b7 to 75e177b Compare April 26, 2019 20:34
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 26, 2019
@baude baude force-pushed the generatesystemd branch from 75e177b to e020d66 Compare April 26, 2019 20:38
@baude baude changed the title [wip] Generate Systemd Generate Systemd Apr 26, 2019
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 26, 2019
@baude
Copy link
Member Author

baude commented Apr 26, 2019

updated and removed the WIP

@baude baude force-pushed the generatesystemd branch from e020d66 to d44eab7 Compare April 26, 2019 20:43
jamescassell
jamescassell reviewed Apr 29, 2019
View reviewed changes
docs/podman-generate-systemd.1.md Outdated
Copy link
Contributor

@jamescassell jamescassell Apr 29, 2019

Choose a reason for hiding this comment

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

Why the change in approach from the simpler example given in podman-run below? i.e., why a pid file and why not the -a option to podman start?

Copy link
Member Author

@baude baude May 2, 2019

Choose a reason for hiding this comment

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

it all has to do with reliability and control of figuring out if the container dies.

@mheon
Copy link
Member

mheon commented Apr 29, 2019 via email

@rhatdan
Copy link
Member

rhatdan commented Apr 29, 2019

I think it would be best if this was template based, that way users could specify a different template that they want to use.

@baude
Copy link
Member Author

baude commented Apr 29, 2019

I think it would be best if this was template based, that way users could specify a different template that they want to use.

I dont know what you mean .... please elaborate.

@rhatdan
Copy link
Member

rhatdan commented Apr 29, 2019

You hard coded a specific unit file format into the tool. I just want you to take that format, make it a template and then allow me to override it.

Then if I want to setup different types of containers running in Unit files, I could do it.

edsantiago
edsantiago suggested changes Apr 29, 2019
View reviewed changes
@rh-atomic-bot
Copy link
Collaborator

rh-atomic-bot commented Apr 30, 2019

☔ The latest upstream changes (presumably #2982) made this pull request unmergeable. Please resolve the merge conflicts.

mheon
mheon reviewed Apr 30, 2019
View reviewed changes
@mheon
Copy link
Member

mheon commented Apr 30, 2019

Two comments and a rebase and this ought to be good

@mheon
Copy link
Member

mheon commented May 2, 2019

@baude Mind looking at this one? Needs a bit more work to make it in

@baude
Generate systemd unit files for containers
c18ad2b 
the podman generate systemd command will generate a systemd unit file
based on the attributes of an existing container and user inputs.  the
command outputs the unit file to stdout for the user to copy or
redirect.  it is enabled for the remote client as well.

users can set a restart policy as well as define a stop timeout
override for the container.

Signed-off-by: baude <bbaude@redhat.com>
@baude baude force-pushed the generatesystemd branch from d44eab7 to c18ad2b Compare May 2, 2019 19:36
@rhatdan
Copy link
Member

rhatdan commented May 2, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 2, 2019
@openshift-merge-robot openshift-merge-robot merged commit b5e5585 into containers:master May 2, 2019
This was referenced May 2, 2019
Merged
Merged
@baude baude deleted the generatesystemd branch December 22, 2019 18:51
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 26, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@rhatdan rhatdan rhatdan left review comments

@mheon mheon mheon left review comments

@umohnani8 umohnani8 Awaiting requested review from umohnani8

+4 more reviewers

@EmilienM EmilienM EmilienM left review comments

@jamescassell jamescassell jamescassell left review comments

@adrianreber adrianreber adrianreber left review comments

@edsantiago edsantiago edsantiago requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

@rhatdan rhatdan

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@baude @openshift-ci-robot @mheon @rhatdan @rh-atomic-bot @edsantiago @EmilienM @jamescassell @adrianreber @openshift-merge-robot