openshift / origin

Conformance test suite for OpenShift
http://www.openshift.org
Apache License 2.0
8.49k stars 4.7k forks source link

Unification of start-build and cancel-build into a single build command #8841

Closed mnagy closed 7 years ago

mnagy commented 8 years ago

We should unify start-build and cancel-build commands into a single build command. It was proposed in the trello card (linked below) to either create subcommands (oc build start and oc build cancel) or use flags, as is currently done for deployments. During a CLI meeting, it was suggested that the subcommands approach should be taken. This however creates an inconsistency with oc deploy which uses flags (--latest, --retry and --cancel). This means that if we chose to go with the subcommand path, we'll need to do a follow-up for deployments.

I think we should also deprecate new-build and add a create buildconfig subcommand instead.

I'd like to get confirmation from @smarterclayton @fabianofranz @jwforres and @bparees that the subcommands are the way to go, or get the discussion going if anyone thinks we should not go by that path, especially from @smarterclayton, as he was the only one out of the four missing on the CLI meeting.

cc @kargakis

Trello: https://trello.com/c/Jo4V19Bf/633-5-unify-build-commands-with-deployments

bparees commented 8 years ago

I think we should also deprecate new-build and add a create buildconfig subcommand instead.

i'm hesitant on this one since mostly create is used for "create exactly this thing" where-as new-build is a much more sophisticated flow that doesn't even necessarily create the resource (ie you can do a dry-run, or output the results to stdout instead of creating the objects).

I could get being a new "new" command with "app" and "build" as sub-commands, but let's defer that discussion for the purposes of this card.

regarding "oc build start" there was some debate on the cli review about how "oc build start" doesn't read as naturally as "oc start-build" so i think we considered keeping aliases around, but one of the benefits of having an "oc build" command with sub-commands is it reduces the number of top level commands and gives users a one stop "oc help build" command to get more information about cli operations related to builds.

smarterclayton commented 8 years ago

We are trying not to add new top level "noun" commands.

On Wed, May 11, 2016 at 12:28 PM, Ben Parees notifications@github.com wrote:

I think we should also deprecate new-build and add a create buildconfig subcommand instead.

i'm hesitant on this one since mostly create is used for "create exactly this thing" where-as new-build is a much more sophisticated flow that doesn't even necessarily create the resource (ie you can do a dry-run, or output the results to stdout instead of creating the objects).

I could get being a new "new" command with "app" and "build" as sub-commands, but let's defer that discussion for the purposes of this card.

regarding "oc build start" there was some debate on the cli review about how "oc build start" doesn't read as naturally as "oc start-build" so i think we considered keeping aliases around, but one of the benefits of having an "oc build" command with sub-commands is it reduces the number of top level commands and gives users a one stop "oc help build" command to get more information about cli operations related to builds.

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/openshift/origin/issues/8841#issuecomment-218513816

bparees commented 8 years ago

We are trying not to add new top level "noun" commands.

can we fail in our trying? or can we say we're using the verb-form of build? :)

Barring that, what's your position on this? Should nothing be done?

smarterclayton commented 8 years ago

I actually want to reserve the verb form of "build" for a replacement for "new-build" and "start-build". I think cancel build is just a different commands structure.

On Wed, May 11, 2016 at 2:59 PM, Ben Parees notifications@github.com wrote:

We are trying not to add new top level "noun" commands.

can we fail in our trying? or can we say we're using the verb-form of build? :)

Barring that, what's your position on this? Should nothing be done?

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/openshift/origin/issues/8841#issuecomment-218556680

bparees commented 8 years ago

can you elaborate on that?

what does "oc build xxxxx" look like when it's used for both "new" and "start"? and is "cancel build" part of the same command structure that would include "cancel deployment"? (personally I don't like the idea of an "oc cancel" command structure because i think "oc help cancel" is going to be less informative/intuitive than "oc help build".. people don't ask "what things can i cancel?" they ask "what can i do with a build?")

smarterclayton commented 8 years ago

I think cancel build should stay cancel-build. It's just as discoverable as it would be under a hypothetical build root command.

On Wed, May 11, 2016 at 3:14 PM, Ben Parees notifications@github.com wrote:

can you elaborate on that?

what does "oc build xxxxx" look like when it's used for both "new" and "start"? and is "cancel build" part of the same command structure that would include "cancel deployment"? (personally I don't like the idea of an "oc cancel" command structure because i think "oc help cancel" is going to be less informative/intuitive than "oc help build".. people don't ask "what things can i cancel?" they ask "what can i do with a build?"

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/openshift/origin/issues/8841#issuecomment-218560536

0xmichalis commented 8 years ago

@smarterclayton we agreed in the cli call last week that consolidating all build commands under one root command should be a thing. I want to switch deploy to subcommands too

smarterclayton commented 8 years ago

I don't think that fits into our upstream strategy.

On Wed, May 11, 2016 at 4:48 PM, Michail Kargakis notifications@github.com wrote:

@smarterclayton https://github.com/smarterclayton we agreed in the cli call last week that consolidating all build commands under one root command should be a thing. I want to switch deploy to subcommands too

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/openshift/origin/issues/8841#issuecomment-218585388

bparees commented 8 years ago

I think cancel build should stay cancel-build. It's just as discoverable as it would be under a hypothetical build root command.

it's not quite as discoverable because it appears in a lengthy list of top level commands which we should strive to make shorter. and it's about answering the question "what are the things i can do with builds?" rather than "what are all the things i can do with oc?"

in any case my discoverability argument was about "oc cancel build" vs "oc build cancel". I agree, to a point, that top level commands are highly discoverable...again, until you have so many of them that the list isn't easily scannable.

smarterclayton commented 8 years ago

It also doesn't fit with other discussions we've had about top level verbs for build and deploy.

On Wed, May 11, 2016 at 5:00 PM, Clayton Coleman ccoleman@redhat.com wrote:

I don't think that fits into our upstream strategy.

On Wed, May 11, 2016 at 4:48 PM, Michail Kargakis < notifications@github.com> wrote:

@smarterclayton https://github.com/smarterclayton we agreed in the cli call last week that consolidating all build commands under one root command should be a thing. I want to switch deploy to subcommands too

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/openshift/origin/issues/8841#issuecomment-218585388

smarterclayton commented 8 years ago

On Wed, May 11, 2016 at 5:01 PM, Ben Parees notifications@github.com wrote:

I think cancel build should stay cancel-build. It's just as discoverable as it would be under a hypothetical build root command.

it's not quite as discoverable because it appears in a lengthy list of top level commands which we should strive to make shorter. and it's about answering the question "what are the things i can do with builds?" rather than "what are all the things i can do with oc?"

Generally a problem that needs to be fixed anyway. Build doesn't make it more discoverable.

in any case my discoverability argument was about "oc cancel build" vs "oc build cancel". I agree, to a point, that top level commands are highly discoverable...again, until you have so many of them that the list isn't easily scannable

Then we fix the top level list and help to help people find their commands.

bparees commented 8 years ago

Generally a problem that needs to be fixed anyway. Build doesn't make it more discoverable.

i would argue it does. all you have to know is "i want build related stuff" and you're on the discovery path, rather than having to discover the specific full command (cancel-build). but certainly it's splitting hairs at some point. again my primary concern is that top level commands should be a carefully curated list and "start-build" and "cancel-build" don't rise to that level imo, just like oc deploy concluded that there shouldn't be a "start-deploy" and "cancel-deploy"

Then we fix the top level list and help to help people find their commands.

meaning what? grouping and indentation can only get you so far.

smarterclayton commented 8 years ago

Eliminating irrelevant commands (or more detailed commands) is also an option.

Upstream is dead set against top level nouns. Let's not pretend that that's not a factor we have to deal with. And as mentioned, build and deploy are excellent verbs for common use cases.

On Wed, May 11, 2016 at 5:08 PM, Ben Parees notifications@github.com wrote:

Generally a problem that needs to be fixed anyway. Build doesn't make it more discoverable.

i would argue it does. all you have to know is "i want build related stuff" and you're on the discovery path, rather than having to discover the specific full command (cancel-build). but certainly it's splitting hairs at some point. again my primary concern is that top level commands should be a carefully curated list and "start-build" and "cancel-build" don't rise to that level imo, just like oc deploy concluded that there shouldn't be a "start-deploy" and "cancel-deploy"

Then we fix the top level list and help to help people find their commands.

meaning what? grouping and indentation can only get you so far.

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/openshift/origin/issues/8841#issuecomment-218590742

mnagy commented 8 years ago

So should I merge new-build and start-build into one build command? Or is this now out of scope of the card?

smarterclayton commented 8 years ago

I'm inclined to wait until we have more discussion.

On May 12, 2016, at 8:04 AM, Martin Nagy notifications@github.com wrote:

So should I merge new-build and start-build into one build command? Or is this now out of scope of the card?

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/openshift/origin/issues/8841#issuecomment-218737231

bparees commented 8 years ago

so the bits i understand so far: oc build is being reserved for verb usage, as in "oc build this thing right now" (where "this thing" could be my local directory, built via binary build).

Whether you could also do something like "oc build bc/foo" which would be the equivalent of "oc start-build bc/foo" is perhaps a question worth asking.

that would still leave us with no good answer for oc cancel-build however.

so given the 4 use cases: 1) run a one-off build based on a repo or directory i specify

2) run an instance of a bc (or rerun a build that's run previously)

3) cancel a running build

4) define a new BC

1,2,4 could be handled via oc build (with (4) being represented by some sort of --buildconfig flag which distinguishes it from a one-off build).

but i still can't reconcile cancel-build into that set of commands in a sensible way.

alternatively we can combine (1) and (4) into the "oc build" command, and have a separate top level command for controlling builds, ie "oc buildctl start foo" and "oc buildctl cancel foo" (not the actual command i'm suggesting, just representational).

but i'd be really happy if we could unify 1-4 under a single top level command, i think. (I say i think because i'm not sure a highly overloaded "build" command is going to be easy for users to understand either)

mnagy commented 8 years ago

@smarterclayton during devexp architecture call last week you were talking about a use-case when a developer just has code and wants to build it and then make iterative changes and rebuild it. It turns out that using our current build commands, this is pretty tedious if you want to also deploy the app to test the code:

# oc new-build --binary --image-stream=openshift/perl:5.20
# oc start-build perl --from-dir=build_test/ --follow
# oc run perl --port=8080 --image=$(oc get is/perl --template '{{.status.dockerImageRepository}}')
# oc expose dc/perl

The oc run will create a deployment, but not with an imageChangeTrigger, so to rebuild and redeploy you have to run oc deploy as well:

# oc start-build perl --from-dir=build_test/ --follow && oc deploy perl --latest

However, using new-app is much simpler and more intuitive:

# oc new-app build_test/
# oc start-build perl --from-dir=build_test/ --follow

This will leave you with BC and Deployment so you can simply rerun the last command to rebuild & redeploy with any changes you made to the code.

So, I think that having oc build as a magic command that does all the stuff for you would have to duplicate what new-app does to be useful, which I think is not what we want. On the other hand, you can always work with new-app and start-build to get the flow of setting up an app and rebuilding quickly.

That said, since new-app and start-build are already sufficient for this use-case, I'd be in favor of just grouping all the build commands under a build noun.

With regards to upstream's verb vs. noun, I found two exceptions (config and rollout) so at least it is not totally out of the question.

One more idea about the first set of commands I mentioned. If we wanted build as one command and it would combine new-build and start-build, we could also modify oc deploy to accept BuildConfigs and create a DC based on them. You could have the same flow with two commands for set-up and one for rebuild & redeploy after making changes to the code. Still no solution for cancel-build, though.

smarterclayton commented 8 years ago

On May 26, 2016, at 5:30 AM, Martin Nagy notifications@github.com wrote:

during devexp architecture call last week you were talking about a use-case when a developer just has code and wants to build it and then make iterative changes and rebuild it. It turns out that using our current build commands, this is pretty tedious if you want to also deploy the app to test the code:

oc new-build --binary --image-stream=openshift/perl:5.20

oc start-build perl --from-dir=build_test/ --follow

oc run perl --port=8080 --image=$(oc get is/perl --template

'{{.status.dockerImageRepository}}')

oc expose dc/perl

The oc run will create a deployment, but not with an imageChangeTrigger, so to rebuild and redeploy you have to run oc deploy as well:

oc start-build perl --from-dir=build_test/ --follow && oc deploy perl --latest

However, using new-app is much simpler and more intuitive:

oc new-app build_test/

oc start-build perl --from-dir=build_test/ --follow

This will leave you with BC and Deployment so you can simply rerun the last command to rebuild & redeploy with any changes you made to the code.

Creating an app before you've had a successful build is not a great experience. It kind of works today, because we make a lot of assumptions about the base image and the Dockerfile. But fundamentally, it is not the right flow. Also, if the build fails, users should not be wondering why the deployment is failed.

The correct experience is:

  1. Run an immediate build and stream it to the console.
  2. If it fails, correct it.
  3. If it succeeds, generate an app for it

That flow should be

$ oc build ... see logs ... $ oc new-app

new-build is the closest, but it is not designed for the flow described above. new-app is not intended to offer all possible build arguments - it can't help users by offering build advice. A big usability blocker is when builds fail - I think it's critical for this command to be reentrant. For example:

$ oc build . ... stuff ... Failure!

$ oc build . The goal is to remove the obstacles to success. Users can't run new-app multiple times, and running start-build is far enough removed from the action that a brand new user has to search to find it. > So, I think that having oc build as a magic command that does all the stuff > for you would have to duplicate what new-app does to be useful, which I > think is not what we want. On the other hand, you can always work with > new-app and start-build to get the flow of setting up an app and rebuilding > quickly. Basically we are talking about repurposing new-build, and putting more limits on what new-app should do. > That said, since new-app and start-build are already sufficient for this > use-case, I'd be in favor of just grouping all the build commands under a > build noun. > > With regards to upstream's verb vs. noun, I found two exceptions (config > and rollout) so at least it is not totally out of the question. It's not, but we'd have to be pretty certain that the outcome was better. The primary desire would be to find a shorter verb that is intuitive to a new user. Would a new user expect "oc build" to do a build? Yes. So when picking the verbs, we want to make that action feel natural. If "oc build" is a top level command, it breaks the natural flow. Another option is to find a command that more closely represents the action for a new user - something like "init" or "new-build" and align them to handle the initial flow. The challenge there is that we'd have to be sure the existing uses of that command are not lost. One more idea about the first set of commands I mentioned. If we wanted build as one command and it would combine new-build and start-build, we could also modify oc deploy to accept BuildConfigs and create a DC based on them. You could have the same flow with two commands for set-up and one for rebuild & redeploy after making changes to the code. Still no solution for cancel-build, though. In theory all of deploys behavior moves to rollout. When that's done, we could repurpose deploy. But that's also much further out.
bparees commented 8 years ago

for now can we replace "oc start-build" with "oc build X" where X can be: 1) bc/foo (a buildconfig) 2) build/foo (a build, so you'd be rebuilding it/rerunning that build) 3) a repo/directory (the one-off build this content now flow) (with this last part being a future implementation, not part of what @mnagy is doing right now to implement the CLI cleanup).

?

we'd retain: 1) oc cancel-build (can't see a way to get rid of it right now) 2) oc new-build (for creating buildconfigs, as opposed to one off builds). this could be lumped in with "3" via a --config or --repeatable argument or something, but for now we don't need to do that.

In this way, "oc build" is always using build as a verb, it's always building something.

The challenge would be that the args that apply to "oc build bc/foo" might not be the same args that apply to "oc build somedir", so that gets a bit messy.

mnagy commented 8 years ago

for now can we replace "oc start-build" with "oc build X" where X can be: 1) bc/foo (a buildconfig) 2) build/foo (a build, so you'd be rebuilding it/rerunning that build) 3) a repo/directory (the one-off build this content now flow) (with this last part being a future implementation, not part of what @mnagy is doing right now to implement the CLI cleanup).

Just to reiterate what was said before off-github: We will have to use flag for the directory case and oc build will have to:

  1. Deduce a name from the directory, preferably using the same code that new-app uses
  2. Look for an image stream with that name and create it if it does not exist

1) oc cancel-build (can't see a way to get rid of it right now)

Should we maybe consider being a little bit less rigid about the whole noun vs. verb debate? I'm not sure having a --cancel flag to oc build would hurt the usability so much, vs. having a special top-level command, unless we find more things that can be cancelled (deployments?).

mnagy commented 8 years ago

@smarterclayton @fabianofranz @jwforres any additional comments? Can I begin the work that @bparees outlined?

yanliao commented 8 years ago

@mnagy Hi, is there any update?

mnagy commented 8 years ago

@yanliao I will raise the topic on tomorrow's CLI meeting and let you know.

bparees commented 7 years ago

tracked in https://trello.com/c/Jo4V19Bf