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

Step "gate" #66

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Step "gate" #66

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e1a326d
WIP: Step "gate"
evanchaoli Jul 28, 2020
ff26835
first good version.
evanchaoli Jul 29, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
133 changes: 133 additions & 0 deletions 064-gate/proposal.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,133 @@
# Summary

This RFC proposes a new step: `gate`.

# Motivation

Concourse pipeline lacks function of conditional workflow. Some use cases are:

1. A pipeline runs code coverage scan. If coverage is lower than 85%, then fail the
build.

2. A pipeline monitors GitLab merged MergeRequests to automatically generate releases,
but if a MR is labeled with "no-release", then silently discard the MR.

For the first use case, there could be two solutions: 1) Add gating function to the
resource type of code coverage scan. This solution is not generic. With this direction,
all similar scan related resource types would have to add gating function; 2) Use a
generic resource type of gating. This solution is better, but using a resource to do
gating is kinda overhead.

For the second use case, gating resource seems to not fit. Because if a resource needs
to abort a build, only way is to fail the build. If a user just wants to silently abort
the build, then gating resource won't work. So that only solution is to make the GitLab
resource to support to filter by MR labels, but which will make GitLab heavier and heavier.
Copy link
Member

Choose a reason for hiding this comment

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

It's true that prototypes will have more code to write to e.g. support filtering by label, but either way there's code to write: if the prototype isn't doing the filtering yourself, it'll need to expose the attributes that can be filtered.

Filtering isn't particularly difficult to implement, so I don't think the difference in code saves a whole lot. I can see the temptation to have the filtering implemented directly in Concourse instead, but I think the challenge would be to achieve UX that's as good or better than this:

var_sources:
- name: prs
  type: github-prs
  source:
    repository: concourse/concourse
    labels: [approved-for-ci]

The topic of generically filtering versions has come up before (concourse/concourse#1176) but I gave up on it myself; I couldn't come up with anything that was a better UX than just configuring the prototype.

Another reason filtering in the prototype helps is that the filtering logic can be very domain-specific: running git log -- <paths>, using a search API, implementing semver range notation, etc. Given that these cases are already common I figured it makes more sense for each prototype to just implement it themselves, since a native implementation can do it either more efficiently or with a better UX than anything wrapping it would likely be able to achieve.


# Proposal

A native step `gate` may make things easier and better.

For the above use case 1, `gate` can be used like:

```yaml
- put: sonar-scan

- load_var: scan-result
file: sonar-scan/result.json

- gate: code-coverage-gate
condition: <a bool expression>

```
Copy link
Member

Choose a reason for hiding this comment

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

This seems like it could be done with a single run step with a configured failure threshold, so I'd be worried about introducing two ways to do the same thing:

- run: check-coverage
  type: go
  params: {threshold: 0.85}

Copy link

Choose a reason for hiding this comment

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

@vito I think one way to think of the difference here is about configuring logic to run as part of the ATC scheduler vs. running the logic inside containers. Needing to pull a container from a registry, then launching the container, for very quick / simple logic turns into a bottleneck very quickly. Being able to express simple logic on the pipeline level can make execution of the logic much faster.

On the other hand, the single run step you propose is "more Concourse-y", as it's very typical for Concourse to launch many copies of small containers for light purposes (this is, after all, what check containers are). But it also contributed to fundamental performance limitations in Concourse's design for a long time, e.g. "dumb" webhooks to trigger checks instead of webhooks whose request bodies could be interpreted.

An RFC like this, to me, seems to try to straddle the line to provide more performant conditional pipeline flows.

Copy link

Choose a reason for hiding this comment

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

I think @vito's idea would be that the container that runs the coverage scan would be what performs the threshold check, rather than spinning up a new container for the comparison (correct me if I'm wrong)


For the above use case 2, `gate` can be used like:

```yaml
- get: mr

- load_var: mr_info
file: mr/mr_metadata.json

- gate: no-release-gate
condition: <a bool expression>
nofail: true
```
Copy link
Member

Choose a reason for hiding this comment

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

One thing I don't like about this approach is that having the build decide not to run based on the properties of an input kind of means the job isn't precisely describing its dependencies. I think it's important to be precise with job inputs so that pipelines are easy to understand - all the way from the UI down to the config.

This could also become wasteful or noisy with certain filtering setups - imagine 90% of your build history are no-op builds which didn't pass the gate. We could address this by hiding the skipped builds or something, but this all kind of feels like it would make jobs feel less meaningful.

It would feel cleaner to me if the filtering was applied directly to the input somehow before a build is created. This way Concourse would only schedule builds for inputs that pass the filters in the first place.


## Step definition

Step `gate` takes a name for description of the gate, and takes the following
parameters:

* `condition` a boolean expression. when `condition` is true, abort the build.
* `nofail` by default, if `condition` is true, the build will fail. If you just
want to silently quit the build (with succeeded result), then set `nofail` to
true.
* `objects` a list of objects defined as:
* `name` object name
* `file` file name to load the object. The file should be in json or yaml format.

## Condition syntax

After some research, I found the package https://github.com/PaesslerAG/gval can
Copy link
Member

Choose a reason for hiding this comment

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

I think there's a risk here of making Concourse's learning curve even steeper by introducing another DSL to learn.

be used for expression evaluation. It supports basic evalutation syntax, and can
be extended to support more operations.

### Integer operations and comparisons

```yaml
- load_var: mr_info
file: mr/data.json

- gate: some-gate
condition: ( ( ((.:mr_info.error)) + ((.:mr_info.failure)) ) / 300 ) > 0.85
```

### String operations and comparisons

```yaml
- load_var: mr_info
file: mr/data.json

- gate: some-gate
condition: "((.:mr_info.result))" == "success"
```

```yaml
- load_var: mr_info
file: mr/data.json

- gate: some-gate
condition: prefix_with( "((.:mr_info.subject))", "WIP" )
```

### JSON object operations

```yaml
- gate: some-gate
condition: "no-release" in mr_info.lables
objects:
- name: mr_info
file: mr/data.json
```


# Open Questions

Any suggestions to condition syntax?

# Answered Questions

> If there were any major concerns that have already (or eventually, through
> the RFC process) reached consensus, it can still help to include them along
> with their resolution, if it's otherwise unclear.
>
> This can be especially useful for RFCs that have taken a long time and there
> were some subtle yet important details to get right.
>
> This may very well be empty if the proposal is simple enough.


# New Implications

> What is the impact of this change, outside of the change itself? How might it
> change peoples' workflows today, good or bad?