Run unit tests as part of build flow

[rhcarvalho]

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:

• https://trello.com/c/BzV509ca/795-8-run-unit-tests-before-pushing-built-images-r-d
• https://trello.com/c/I1v1YeOf/844-8-implement-post-build-command

---

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

• User enter one or more lines in a text area 👍
  • In drone.io, you need to include all the steps required before the command that run tests (install systems packages, dependencies, etc) 👎
  • In OpenShift, we don't need those extra steps, npm test would be enough 👍
• 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 👍
• Choosing a language automatically fill in the "build command" with some default value for the given language 👍
• Commands are run in Bash (mentioned in their intro video, but a written specification could not be found in their official docs)

Circle CI

• User enter one or more lines in a text area 👍
• It is clearly specified that tests will be run with Bash on Ubuntu 12.04 (make it a predictable experience) 👍
• 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 👎
• Allows specifying, in a concise way, multiple scripts that are sequentially executed even if a previous command returns non-zero exit code 👍
• "Each command in script is processed by a special bash function." (predictable experience) 👍

[rhcarvalho]

@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]

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]

@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]

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]

@jwforres 👍 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 😄 )

[mfojtik]

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

[bparees]

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

[jwforres]

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 😄

[rhcarvalho]

I'm thinking a "reason" field

Fortunately it already exists, we'd just need to add more possible values:

origin/pkg/build/api/v1/types.go

Line 61 in 2670ca0

Reason StatusReason `json:"reason,omitempty" description:"brief CamelCase string describing a failure, meant for machine parsing and tidy display in the CLI"`

[rhcarvalho]

@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]

@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]

"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]

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]

@bparees +1

[rhcarvalho]

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]

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]

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]

@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]

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]

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]

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]

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]

@bparees +1

[smarterclayton]

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]

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]

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]

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]

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]

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]

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]

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 #6758 via #6715
#6715.

—
Reply to this email directly or view it on GitHub
#6758 (comment).

[bparees]

@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]

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

[spadgett]

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]

@spadgett docs will hopefully be ready today/tomorrow openshift/openshift-docs#1448

[rhcarvalho]

@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).