Run unit tests as part of build flow

rhcarvalho commented 8 years ago

We want to support developers and teams using OpenShift to implement CI/CD workflows.

We want to close gaps and offer seamless transitions from simply running unit tests to more complex scenarios including integration with Jenkins.

This issue is meant to discuss how we add support for running unit tests as part of the build flow.

Requirements:

Should require a minimum amount of user input, no boilerplate (think rake test);
Should require no changes to source repositories (solutions like .travis.yml files are discarded);
Should work the same for Source and Docker builds;
If the test execution returns a non-zero exit code, the build will be marked as failed;
Logs shall be streamed to the build logs, just like S2I streams logs from the assemble script;
It should be possible to install test-only dependencies as part of the test script, and these dependencies will not be committed to the final image.

Related Trello cards:

Assumptions

The unit tests will be executed on a fresh container running the Docker image that has been built, right after the last commit and prior to pushing to a registry (think docker run image@sha test command);
The working directory is not changed, so the tests will be executed in the default working directory of the image;
For this test command to work, assume that source code or whatever other fundamental artifacts are present in the image.
Test execution time is not limited, nor is the scope of what the test script can do. In fact it could be abused to do anything a bash script can do. Timeouts, and other resource quota related concerns are taken care by the platform, and the user uses her quota the way she wants.
How existing CI tools work w.r.t. running unit tests

Drone.io

droneio-cmd

User enter one or more lines in a text area :+1:
- In drone.io, you need to include all the steps required before the command that run tests (install systems packages, dependencies, etc) :-1:
- In OpenShift, we don't need those extra steps, npm test would be enough :+1:
It is possible to pass in extra environment variables (but, instead of this, perhaps users could add lines export FOO=bar in their list of commands to run?)
The working directory where the command is run is clearly shown in the UI :+1:
Choosing a language automatically fill in the "build command" with some default value for the given language :+1:
Commands are run in Bash (mentioned in their intro video, but a written specification could not be found in their official docs)
Circle CI

circleci-cmd-test

User enter one or more lines in a text area :+1:
It is clearly specified that tests will be run with Bash on Ubuntu 12.04 (make it a predictable experience) :+1:
If user don't provide their own script, Circle CI tries to infer the commands based on the software stack
Travis

https://docs.travis-ci.com/user/customizing-the-build/#Customizing-the-Build-Step

script:
  - bundle exec rake build
  - bundle exec rake builddoc

Requires writing a .travis.yml file :-1:
Allows specifying, in a concise way, multiple scripts that are sequentially executed even if a previous command returns non-zero exit code :+1:
"Each command in script is processed by a special bash function." (predictable experience) :+1:

rhcarvalho commented 8 years ago

@bparees @mfojtik @PI-Victor @smarterclayton @liggitt @deads2k

I'd like to take the API discussion from #6715 to this issue, and also include the @openshift/ui-review team, since to effectively achieve our objective of making it easy to include unit tests as part of a build, UI work will have to be done, as that's what end users will mostly interact with.

deads2k commented 8 years ago

I don't see unit tests as part of a build. I see the build completing, pushing to its image stream, then having that trigger a series of jobs that are created to represent the various tests. Upon successful completion of the tests, the imagestreamtag is annotated with various pass indicators and maybe promoted by re-tagging.

That flow fits with most systems I've worked with. The build and its output is considered successful regardless of test passes. Then a series of different tests are used to vet the build, adding information until various boundaries are passed that allow promotion to addition test activity.

You could even use jobs to do the work we have in our test queue, where we have multiple test attempts and all of them have to pass.

bparees commented 8 years ago

@deads2k that ship has sailed (during extensive discussions about what this feature/card is for) so please don't derail this discussion. The feature is: image has been built and committed but not pushed, I want to run a command against the image to perform some basic unit test/validation. I do not want to have to push the image to a repository and deploy it somewhere and do complicated things.

jwforres commented 8 years ago

It should be clear from something in the API response for the build that the reason for the build failure was the unit tests step, and not that the build itself failed, preferably not just a block of status details text, I'm thinking a "reason" field? You may want to visually flag these failures differently in the UI.

rhcarvalho commented 8 years ago

@jwforres :+1: I'd love to see builds that failed unit tests "colored" differently -- there are a few "new" cases to consider:

failed to run unit tests (because of configuration errors, typos, etc)
unit tests passed
unit tests failed
unit tests were not run (with a link to setup unit tests for the build config :smile: )

mfojtik commented 8 years ago

@rhcarvalho are you going to update the build annotation to deliver the message to UI?

bparees commented 8 years ago

@rhcarvalho now who's feature bloating.....

jwforres commented 8 years ago

So we won't limit their execution time, but should we allow them to set a "kill my build unit tests if they take longer than X amount of time". Only reason would be that quota-ed users may not want to block their whole build queue with one borked test run. Obviously they could do this themselves in their test commands, but having a guarantee from the system that they will get killed can be comforting :smile:

rhcarvalho commented 8 years ago

I'm thinking a "reason" field

Fortunately it already exists, we'd just need to add more possible values: https://github.com/openshift/origin/blob/2670ca01a09977a13e0b3583ede3cd855ad705dc/pkg/build/api/v1/types.go#L61

rhcarvalho commented 8 years ago

@bparees Web Console work are ideas for @jwforres to make we recognized by excellent UX. Of couse it doesn't affect the scope of my PR that will introduce the feature.

bparees commented 8 years ago

@rhcarvalho i'm not really clear on the point of this issue. If you want to drive the discussion about what the api for defining the test-cmd should be, let's do that in a specific discussion, not one that revisits the entire feature. (The UX discussion is a fine one to have, but that doesn't seem like your most pressing concern for getting the card done, right?)

rhcarvalho commented 8 years ago

"kill my build unit tests if they take longer than X amount of time"

@jwforres there's already oc cancel-build today, so adding a button to the UI to cancel an in-flight build is certainly possible. We're not aiming to add automatic cancel after a timeout.

jwforres commented 8 years ago

yeah @rhcarvalho we already have cancel build in the UI, this was from the perspective of my build queue is running overnight with no one watching it so nothing was able to finish. But yep that's fine, was just a suggestion.

mfojtik commented 8 years ago

@bparees +1

rhcarvalho commented 8 years ago

what the api for defining the test-cmd should be

So, when we started to talk about container API, and modelling this after ExecNewPodHook (Deployment Config lifecycle hooks), I think things started to look more complex then they need to be, based on the scope originally discussed.

We have previously aborted Build lifecycle hooks #3674, which were modeled after DC hooks. The implementation was exactly in that direction -- generic fields that let you run any image with any argument, entrypoint, volumes, env, etc, etc. It was super powerful, could be used to cover many use cases, but still solved no clear use case in a very easy to use way.

Our next step was to target something more focused -- Start builds after a build completes #6311. It have clearly a much more focused scope and all the API suggestions targeted solving a single problem. We put there the elements that we needed, nothing more, and still kept in mind we might add more things in the future. And eventually we aborted it.

This is the third try at solving a very particular use case. I wouldn't be surprised that at this time our implementation is even less generic, but made to solve one problem very well and leaving room for additive changes.

All CI tools we looked at give you a single free-form string field that is run as a bash script. That is sufficient to all unit testing use cases we've discussed. That makes users happy, they know what they get. Advanced users can do arbitrary things, combine commands with &&, get rid of bash by using exec, and so on.

I've also seen people messing up with passing string arrays to cmd and entrypoint. Sure we could do that, but why complicate?

rhcarvalho commented 8 years ago

apiVersion: v1
kind: BuildConfig
metadata:
  name: example
  namespace: demo
spec:
  output:
    to:
      kind: ImageStreamTag
      name: example:latest
  resources: {}
  source:
    git:
      uri: https://github.com/openshift/example
    type: Git
  strategy:
    dockerStrategy: {}
    type: Docker
  # postCommit: specify commands to be run in the output Docker image after its
  # last layer is committed and before the image is pushed to a registry. Use
  # this to run unit tests.
  postCommit:
    # runBash: most people will be fine with running their unit tests within a
    # Bash script, as that's what they are used to in tools they use today. Bash
    # scripts are flexible, and can be simple and powerful. While it is easy to
    # call 'rake test', you're not limited in any way to perform more advanced
    # actions if need be.
    runBash: rake test
    # run: if there really are interesting user-driven use cases where the image
    # entrypoint needs to be preserved, or that the user needs to explicitly
    # specify a custom entrypoint, then she uses "run" and not "runBash".
    run:
      # entrypoint: if omitted, the image's entrypoint will be preserved. Maps
      # directly to the entrypoint option when starting a Docker container.
      entrypoint: ["custom", "value"]
      # cmd: maps directly to the cmd option when starting a Docker container.
      cmd: ["custom", "value"]
      # if need be, this can grow to match the Docker Container spec, including
      # 'env', 'volumes', etc.

What I am proposing is postCommit.runBash for today, because that is simple, direct, and predictable and is what we have use cases for, while having the possibility to have postCommit.run in the future if need be. Unless anyone can present a use case today that would not be addressed by the proposed solution.

0xmichalis commented 8 years ago

image has been built and committed but not pushed

I don't think @deads2k described something complicated and out of scope. I mentioned it before to @mfojtik, a job would be ideal for running my unit tests. Once indexed jobs are implemented, they may be even more valuable for running tests in parallel. I wouldn't promote the image in case of failed unit tests though.

bparees commented 8 years ago

@kargakis i'll relay you the same points i made to @deads2k

(09:51:39 AM) bparees: "i want to run rake test as part of my build process" (09:52:03 AM) bparees: i do not want to setup a deploymentconfig/job definition and all the other stuff required to do that, check results, do more promotion flows, etc. (09:53:13 AM) bparees: and how do i promote an image as a result of a successful job execution? (09:53:16 AM) bparees: how do i chain jobs? (09:53:23 AM) bparees: how do i go from build->job->deployment? (09:53:53 AM) bparees: how do i easily see the success/failure result of those jobs as associated with a particular build i ran?

The fact that there might be a way to kick off a job based on a build completing does not solve those stories, and it still requires me to know how to go define yet another api object and tie it together with something.

Build(success)->Deployment is a well defined flow in the product today that has a complete UX around it.

Build->Job execution->Deployment is not and we are not signing up to solve that flow/UX with this card.

liggitt commented 8 years ago

What I am proposing is postCommit.runBash for today, because that is simple, direct, and predictable and is what we have use cases for, while having the possibility to have postCommit.run in the future if need be.

Let's not plan to have two parallel way to describe running a command. I think @smarterclayton's comments at https://github.com/openshift/origin/pull/6715/files#r50259107 and https://github.com/openshift/origin/pull/6715/files#r50259303 stand

0xmichalis commented 8 years ago

how do i go from build->job->deployment

I would guess some kind of a post-hook mechanism in builds that would push the image on successful completion.

bparees commented 8 years ago

I would guess some kind of a post-hook mechanism in builds that would push the image on successful completion.

so i've got to have a buildconfig that explicitly starts a job, then waits for that job to complete before taking a next action? that's definitely complicated and out of scope relative to what we are doing here.

also as i mentioned to @deads2k when we discussed this offline, you require additional quota if you're going to launch additional pods. This approach does not require additional quota from the user because we know we're running operations in serial and can use your quota serially.

bparees commented 8 years ago

Let's not plan to have two parallel way to describe running a command. I think @smarterclayton's comments at https://github.com/openshift/origin/pull/6715/files#r50259107 and https://github.com/openshift/origin/pull/6715/files#r50259303 stand

I would mostly agree except that we are not doing an Exec, therefore using the ExecAction api directly as @smarterclayton suggests is going to be misleading to users, particularly if they read the docs which state "The command is simply exec'd, it is not run inside a shell, so traditional shell instructions ('|', etc) won't work."

Otherwise i'd agree that the ExecAction structure is fine. For 99% of our users, who will be using our s2i images, the existing entrypoint plus their "rake test" cmd[] will get exactly the behavior they want, in a simple way and we don't need to do anything else immediately.

the next thing i'd expect us to need to add would be support for an explicit entrypoint, primarily to handle a scenario like "i'm doing a docker strategy build of a jenkins image. my dockerfile sets the ENTRYPOINT to "java -jar jenkins.jar", but i also want to run a post-build command to check some stuff in the image. which means i need to override the ENTRYPOINT so it doesn't just start jenkins and ignore my test commands".

But even if we don't offer that, they can do what everyone else does and have their ENTRYPOINT be a shell script that, for no arguments, runs "java -jar jenkins.jar" and when invoked with arguments, runs a shell with those arguments.

jhadvig commented 8 years ago

@bparees +1

smarterclayton commented 8 years ago

Jobs are not the right solution. A build config is a logical chunk of work, and build configs resulting in jobs being created might be a future enhancement, but not today.

smarterclayton commented 8 years ago

Entry point is not needed and would be an antipattern here (it's the image run statement, not a hook). The exec action struct is not critical, but the similarity in shape is. I would probably phrase it as a subclass / variant of ExecAction.

rhcarvalho commented 8 years ago

IIUC what @smarterclayton is saying, I agree with the fact that setting an entrypoint is an antipattern, as I haven't seen a good reason not to document and always use "bash -c" yet. My initial inclusion of "Bash" in the name of the field was aimed at helping users identify and understand what they are supposed to write in their templates, and to understand at first sight when they read a template (what will happen more often), without having to RTFM all the time.

the next thing i'd expect us to need to add would be support for an explicit entrypoint, primarily to handle a scenario like "i'm doing a docker strategy build of a jenkins image. my dockerfile sets the ENTRYPOINT to "java -jar jenkins.jar", but i also want to run a post-build command to check some stuff in the image. which means i need to override the ENTRYPOINT so it doesn't just start jenkins and ignore my test commands".

@bparees IMHO this wouldn't be a problem if we had a fixed entrypoint. No conditionals, no multiple paths and possibilities to document, less confused users.

If we can make it work for all images that contain bash, why limit ourselves to images that set the entrypoint to "something convenient"?!

But even if we don't offer that, they can do what everyone else does and have their ENTRYPOINT be a shell script that, for no arguments, runs "java -jar jenkins.jar" and when invoked with arguments, runs a shell with those arguments.

I don't think we can/should force users to change the way they build their images / write their Dockerfiles just to satisfy our API to run unit tests.

Let's not plan to have two parallel way to describe running a command

@liggitt, the "future" I mentioned in my previous comment/proposal might simply never happen. That's my Agile/eXtreme Programming brain at work. Perhaps it's not a good fit here.

I contrasted runBash with a generic run just to show that one doesn't exclude the other. If what everyone gets today is a bash script in Drone.io, Circle CI, Travis, and we're all fine and productive with it, why shall we complicate the matters by offering more choices?

I want a PaaS that does what I need in a concise and easy to understand way, not one that has millions of features that I don't understand how to use.

I'm going to update the PR based on the feedback so far. Will be making it look more like ExecAction.

rhcarvalho commented 8 years ago

the "future" I mentioned in my previous comment/proposal might simply never happen. That's my Agile/eXtreme Programming brain at work. Perhaps it's not a good fit here.

Maybe this was too broad a comment. What I had in mind specifically was YAGNI: https://en.wikipedia.org/wiki/You_aren%27t_gonna_need_it

mfojtik commented 8 years ago

Agree with @bparees, the ExecAction structure is fine for the actual use-case. We can copy it to our type ExecPostCommitAction and provide better documentation. We don't have to override entrypoint (but we can add that in future, where there will be a use-case for that) as the entrypoint we use for s2i image is already bash (and upstream builder images will presumably follow that pattern).

rhcarvalho commented 8 years ago

For Docker builds, I expect the Dockerfile to set entrypoint and/or cmd such that running a container off the image will "run the app": start a database server, web server, etc. Our assumption was that people will not have to change their source repo (with their Dockerfile) in order to use this feature.

Running a container with user-provided input as Docker's "cmd" and using whatever entrypoint is in the image does not solve this use case IHMO.

bparees commented 8 years ago

Running a container with user-provided input as Docker's "cmd" and using whatever entrypoint is in the image does not solve this use case IHMO.

i'm sticking with "if that becomes a common use case, we will add an entrypoint field to our ExecAction struct".

This is how I see the scenarios: 1) image contains a proper entrypoint like our s2i images - cmd[] works great 2) image contains no entrypoint, runs a CMD instead. - cmd[] works reasonably well (may need to specifying bash -c as part of the cmd, depending what you are doing) because we just ignore their CMD. 3) image contains an ENTRYPOINT that does bad things (just starts the DB, doesn't look at other args). - we add an entrypoint override field in the future.

smarterclayton commented 8 years ago

Sweet, now we just need the CLI for setting it

On Mon, Feb 15, 2016 at 2:31 PM, OpenShift Bot notifications@github.com wrote:

Closed #6758 https://github.com/openshift/origin/issues/6758 via #6715 https://github.com/openshift/origin/pull/6715.

— Reply to this email directly or view it on GitHub https://github.com/openshift/origin/issues/6758#event-551123230.

bparees commented 8 years ago

@smarterclayton https://trello.com/c/YpZLKSAX/790-3-add-ability-to-specify-pre-post-deployment-hooks-as-part-of-new-app-evg

but likely won't drop this sprint (or for 3.2 in general)

rhcarvalho commented 8 years ago

@jwforres FYI this has been merged, would be nice to have in the Web Console as well to make it easy to use.

spadgett commented 8 years ago

FYI this has been merged, would be nice to have in the Web Console as well to make it easy to use.

@rhcarvalho A lot of history here and in the PR. Any doc or summary of how it works?

PI-Victor commented 8 years ago

@spadgett docs will hopefully be ready today/tomorrow https://github.com/openshift/openshift-docs/pull/1448

rhcarvalho commented 8 years ago

@spadgett apart from the docs link above, also please look at the screenshots in the original issue description. We want to make sure it's easy to specify and edit the command to be run, and that the working directory is clear. In case of using the script form, we should also make clear that it will be run using /bin/sh -c (so no Bash-only syntax, and the image needs to have the /bin/sh binary).

openshift / origin

Run unit tests as part of build flow #6758

Assumptions

How existing CI tools work w.r.t. running unit tests

Drone.io

Circle CI

Travis