add oc create dc #8712

deads2k · 2016-05-02T19:24:53Z

Adds oc create dc so I can do oc create dc my-nginx --image=nginx -- nginx -t -c /path/to/mynginx.conf so I can easily write the service serving cert test for clayton.

@Kargakis @stevekuznetsov

0xmichalis · 2016-05-02T19:49:11Z

Can't you use oc run?

deads2k · 2016-05-02T20:37:34Z

Can't you use oc run?

Counting on the output of an intent-like command like oc run doesn't make a lot of sense. I want to create a DC and I want to be sure that I get a DC. I don't want to be trying to find the right generator or trying to pin to various versions to avoid getting broken the next time it changes.

0xmichalis · 2016-05-03T09:24:53Z

to avoid getting broken the next time it changes.

That's the concept of pinning down to a generator. I haven't seen anyone complaining about the current state with respect to creating deployments (not saying that it's optimal). Also, why do you need a dc for your test? Can't you run a pod or rc? Will you test deployments?

@fabianofranz @smarterclayton what's your opinion on this? Do we want to support duplicate commands? We agreed that we don't want to continue adding to oc expose so we created oc create route for generating secured routes which is fine. I don't see any difference though between oc run --generator=deploymentconfig/v1 and oc create dc. Are we planning on moving anything that generates resources under oc create?

deads2k · 2016-05-03T11:54:29Z

Do we want to support duplicate commands?

These aren't duplicate commands.

One is oc run that means (from a user perspective): run this image somehow with all these flags that I'm not likely to understand and have different affects on different generators (what's a generator again?) and thus always have broken documentation. I think when a CLI command passes 30 local flags (not even considering generator variation), its time to rethink your strategy and your expectations about users actually using it.

The other is oc create dc that means: I want a DC for this image and command, I'll sort out the rest using oc set if I have to, but I don't want to be authoring yaml, make it for me.

0xmichalis · 2016-05-03T12:46:51Z

pkg/cmd/cli/cmd/create/deploymentconfig.go

+	cmd := &cobra.Command{
+		Use:     name + " NAME --image=IMAGE -- [COMMAND] [args...]",
+		Short:   "Create deployment config with default options that uses a given image.",
+		Long:    policyBindingLong,


deploymentConfigLong

stevekuznetsov · 2016-05-03T13:34:40Z

With only one flag and lots of defaulting, are we sure this is going to be useful? Maybe it would be better to have a "default" target on the generator?

deads2k · 2016-05-03T14:18:26Z

With only one flag and lots of defaulting, are we sure this is going to be useful? Maybe it would be better to have a "default" target on the generator?

There are no generators for origin resources. Turns out that most of the time you're starting with this, you don't care about the vast majority of things we can set. Even just getting the yaml skeleton so oc edit works has significant value.

deads2k · 2016-05-03T14:35:44Z

comments addressed.

0xmichalis · 2016-05-03T14:50:46Z

pkg/cmd/cli/cmd/create/deploymentconfig.go

+	o := &CreateDeploymentConfigOptions{Out: out}
+
+	cmd := &cobra.Command{
+		Use:     name + " NAME --image=IMAGE -- [COMMAND] [args...]",


I don't see any special handling of dashes. You may want to have a look at kubernetes/kubernetes#13917

I don't see any special handling of dashes. You may want to have a look at kubernetes/kubernetes#13917

Added the check to make sure that you only have one arg before the dash.

0xmichalis · 2016-05-03T14:53:57Z

cc: @fabianofranz @ironcladlou

smarterclayton · 2016-05-03T16:59:49Z

Please concretely explain why this is not oc run.

deads2k · 2016-05-03T17:12:40Z

Please concretely explain why this is not oc run.

I know that I want a DC. I will always want a DC. I will never want this command to decide to make a job or a pod or an RC or a D or any of the other things that have broken what it does in the past. I the simplest possible command with options that I can understand that always behave predictably to get me started without having to know yaml and allow easy composition with oc set.

This is exactly the command that I wanted to start the nginx stuff because I knew what I wanted and couldn't figure out how to use oc run.

smarterclayton · 2016-05-03T17:43:32Z

If the args for your command look like run, you should be using run. If you don't trust run, that's reasonable, but the answer is to use run and we should fix your worries. I do not want to add a create deploymentconfig command that offers nothing outside of run. We already have plans to add something that is focused on deploying images, I'd prefer not to add a third way.

deads2k · 2016-05-03T17:53:08Z

If the args for your command look like run, you should be using run. If you don't trust run, that's reasonable, but the answer is to use run and we should fix your worries. I do not want to add a create deploymentconfig command that offers nothing outside of run. We already have plans to add something that is focused on deploying images, I'd prefer not to add a third way.

Unless we undo 90% of what oc run is doing, its never going to be the "you know what resource you want, but can't author the yaml, start here" command. As we continue have more easy oc create commands, this problem is going to get worse and the fact that DCs will be missing is going to look like a glaring oversight. Pointing someone at a giant command that isn't focused on what they're doing and can't realistically be fixed is a terrible way to try to get started doing something more tailored than oc new-app. It makes the gulf from "I know nothing" to "I know a little bit" a giant impassible chasm that I can't cross.

deads2k · 2016-05-03T19:43:22Z

So I can find this in the future, all you need to do is alias 'oc-create-dc'="oc run --restart=Always --expose=false --attach=false --generator=deploymentconfig/v1 --record=false --save-config=false" and you'll have enough stability in oc run to use it, probably. Can't imagine why I didn't think of that first.

smarterclayton · 2016-05-03T21:55:58Z

Such snark. As we talked about in person I'm ok with this if you can describe the boundaries clearly in doc about why these exist, and what the limit is on what people can add to them (probably in kubectl conventions upstream).

smarterclayton · 2016-05-03T21:56:33Z

Or: alias 'oc-create-dc'="oc run ', which is the same.

smarterclayton · 2016-05-04T04:36:32Z

pkg/cmd/cli/cmd/create/deploymentconfig.go

+	DeploymentConfigRecommendedName = "deploymentconfig"
+
+	deploymentConfigLong = `
+Create a deployment config that uses a given image.`


Describe what deployment configs are and why I want them. Crib from the API doc if necessary.

Describe what deployment configs are and why I want them. Crib from the API doc if necessary.

added.

deads2k · 2016-05-04T19:20:58Z

Upstream pull got agreement in principle for kubectl create <subcommand>

I think I got all the comments.

deads2k · 2016-05-04T23:48:21Z

pkg/cmd/cli/cmd/create/deploymentconfig.go

+
+func (o *CreateDeploymentConfigOptions) Complete(cmd *cobra.Command, f *clientcmd.Factory, args []string) error {
+	argsLenAtDash := cmd.ArgsLenAtDash()
+	if argsLenAtDash != 1 {


note to self, handle the case of no dashes

deads2k · 2016-05-05T19:49:50Z

any other comments?

0xmichalis · 2016-05-05T19:57:08Z

lgtm [merge]

openshift-bot · 2016-05-05T20:00:39Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/5827/) (Image: devenv-rhel7_4118)

openshift-bot · 2016-05-05T20:00:39Z

Evaluated for origin merge up to 0bde6e5

openshift-bot · 2016-05-05T20:00:40Z

[Test]ing while waiting on the merge queue

openshift-bot · 2016-05-05T20:05:27Z

Evaluated for origin test up to 0bde6e5

openshift-bot · 2016-05-05T21:04:23Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/3629/)

fabianofranz · 2016-05-06T17:50:30Z

LGTM, pretty much what I meant here.

0xmichalis self-assigned this May 3, 2016

0xmichalis added the component/cli label May 3, 2016

0xmichalis reviewed May 3, 2016
View reviewed changes

deads2k force-pushed the create-dc branch from cf6d90f to 0cad48c Compare May 3, 2016 14:35

0xmichalis reviewed May 3, 2016
View reviewed changes

deads2k closed this May 3, 2016

smarterclayton reopened this May 3, 2016

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 4, 2016

smarterclayton reviewed May 4, 2016
View reviewed changes

deads2k force-pushed the create-dc branch from 0cad48c to 064c151 Compare May 4, 2016 19:06

deads2k force-pushed the create-dc branch from 064c151 to ce8e437 Compare May 4, 2016 19:20

deads2k force-pushed the create-dc branch from ce8e437 to 5f2b7f0 Compare May 4, 2016 19:26

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 4, 2016

deads2k reviewed May 4, 2016
View reviewed changes

add oc create dc

0bde6e5

deads2k force-pushed the create-dc branch from 5f2b7f0 to 0bde6e5 Compare May 5, 2016 19:49

openshift-bot merged commit 2bce3bd into openshift:master May 6, 2016

deads2k deleted the create-dc branch September 6, 2016 17:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add oc create dc #8712

add oc create dc #8712

deads2k commented May 2, 2016

0xmichalis commented May 2, 2016

deads2k commented May 2, 2016

0xmichalis commented May 3, 2016

deads2k commented May 3, 2016

0xmichalis May 3, 2016

stevekuznetsov commented May 3, 2016

deads2k commented May 3, 2016

deads2k commented May 3, 2016

0xmichalis May 3, 2016

deads2k May 4, 2016

0xmichalis commented May 3, 2016

smarterclayton commented May 3, 2016

deads2k commented May 3, 2016

smarterclayton commented May 3, 2016

deads2k commented May 3, 2016

deads2k commented May 3, 2016

smarterclayton commented May 3, 2016

smarterclayton commented May 3, 2016

smarterclayton May 4, 2016

deads2k May 4, 2016

deads2k commented May 4, 2016

deads2k May 4, 2016

deads2k commented May 5, 2016

0xmichalis commented May 5, 2016

openshift-bot commented May 5, 2016 •

edited

openshift-bot commented May 5, 2016

openshift-bot commented May 5, 2016

openshift-bot commented May 5, 2016

openshift-bot commented May 5, 2016

fabianofranz commented May 6, 2016

add oc create dc #8712

add oc create dc #8712

Conversation

deads2k commented May 2, 2016

0xmichalis commented May 2, 2016

deads2k commented May 2, 2016

0xmichalis commented May 3, 2016

deads2k commented May 3, 2016

0xmichalis May 3, 2016

Choose a reason for hiding this comment

stevekuznetsov commented May 3, 2016

deads2k commented May 3, 2016

deads2k commented May 3, 2016

0xmichalis May 3, 2016

Choose a reason for hiding this comment

deads2k May 4, 2016

Choose a reason for hiding this comment

0xmichalis commented May 3, 2016

smarterclayton commented May 3, 2016

deads2k commented May 3, 2016

smarterclayton commented May 3, 2016

deads2k commented May 3, 2016

deads2k commented May 3, 2016

smarterclayton commented May 3, 2016

smarterclayton commented May 3, 2016

smarterclayton May 4, 2016

Choose a reason for hiding this comment

deads2k May 4, 2016

Choose a reason for hiding this comment

deads2k commented May 4, 2016

deads2k May 4, 2016

Choose a reason for hiding this comment

deads2k commented May 5, 2016

0xmichalis commented May 5, 2016

openshift-bot commented May 5, 2016 • edited

openshift-bot commented May 5, 2016

openshift-bot commented May 5, 2016

openshift-bot commented May 5, 2016

openshift-bot commented May 5, 2016

fabianofranz commented May 6, 2016

openshift-bot commented May 5, 2016 •

edited