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.

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

Add an option to control if play kube should start the pod #8363

Merged
merged 1 commit into from
Nov 17, 2020
Merged

Add an option to control if play kube should start the pod #8363

merged 1 commit into from
Nov 17, 2020

Conversation

AlbanBedel
Copy link
Contributor

Having play kube start the pod is not always appropriate, one might
for example like to have the pod running as a set of systemd services.
This new option allow to skip starting the pod.

TomSweeneyRedHat
TomSweeneyRedHat reviewed Nov 17, 2020
View reviewed changes
docs/source/markdown/podman-play-kube.1.md Outdated
@@ -64,6 +64,10 @@ Require HTTPS and verify certificates when contacting registries (default: true)
then TLS verification will be used. If set to false, then TLS verification will not be used. If not specified,
TLS verification will be used unless the target registry is listed as an insecure registry in registries.conf.

#### **--create-only**, **-c**
Copy link
Member

Choose a reason for hiding this comment

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

move this up to line 38 so the options will be listed alphabetically (with the exception of help)

@TomSweeneyRedHat
Copy link
Member

@AlbanBedel you've a lint error and I don't think you signed your PR which our CI requires (git commit --amend -s will take care of the signature).

Otherwise the change LGTM

Luap99
Luap99 reviewed Nov 17, 2020
View reviewed changes
Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

This also has to be wired into the api and remote client.

Server logic here: pkg/api/handlers/libpod/play.go
Client logic here: pkg/bindings/play/play.go
API docs here: pkg/api/server/register_play.go

@rhatdan
Copy link
Member

rhatdan commented Nov 17, 2020

How about the option be --now, similar to systemctl enable service --now.

The default is true, to be backwards compatible and --now=false, gives you this functionality.

@AlbanBedel
Copy link
Contributor Author

How about the option be --now, similar to systemctl enable service --now.

The default is true, to be backwards compatible and --now=false, gives you this functionality.

In the context of a command named play I would find --now a bit confusing without further explanations. What about --start-now=false?

@rhatdan
Copy link
Member

rhatdan commented Nov 17, 2020

I am not a big fan of options with a "-" inside of them, which is one of the reasons, I wanted to make the change.

@AlbanBedel AlbanBedel force-pushed the play-kube-create-only branch from b522fa2 to 3ed2382 Compare November 17, 2020 18:54
@AlbanBedel
Add an option to control if play kube should start the pod
7ab936e 
Having play kube start the pod is not always appropriate, one might
for example like to have the pod running as a set of systemd services.
Add a `start` option to the command line and API to control if the pod
should be started or not; it defaults to true for backward
compatibility.

Signed-off-by: Alban Bedel <[email protected]>
@AlbanBedel AlbanBedel force-pushed the play-kube-create-only branch from 3ed2382 to 7ab936e Compare November 17, 2020 19:02
@AlbanBedel AlbanBedel changed the title Add a --create-only option to play kube Add an option to control if play kube should start the pod Nov 17, 2020
@AlbanBedel
Copy link
Contributor Author

In this new version:

  • Renamed the option to start as I find now to not be self explanatory enough
  • Added the new option to the client, server and API documentation
  • Fixed the lint errors
  • Moved the man page entry to the right position
  • Reworked the commit message

@TomSweeneyRedHat The commit has my SOB, I just removed it from the PR description.

@rhatdan
Copy link
Member

rhatdan commented Nov 17, 2020

LGTM
Thanks @AlbanBedel

@TomSweeneyRedHat
Copy link
Member

The ubuntu-19 root tests aren't happy. Not sure if it's a known issue or not.

@TomSweeneyRedHat
Copy link
Member

LGTM
assuming happy tests

@rhatdan
Copy link
Member

rhatdan commented Nov 17, 2020

/approve
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 17, 2020
@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AlbanBedel, rhatdan

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

The pull request process is described here

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 Nov 17, 2020
@openshift-merge-robot openshift-merge-robot merged commit 770b03a into containers:master Nov 17, 2020
@edsantiago edsantiago mentioned this pull request Nov 18, 2020
Closed
@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 24, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Luap99 Luap99 Luap99 left review comments

@TomSweeneyRedHat TomSweeneyRedHat TomSweeneyRedHat left review comments

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.
6 participants
@AlbanBedel @TomSweeneyRedHat @rhatdan @openshift-ci-robot @Luap99 @openshift-merge-robot