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.

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

wants to merge 2 commits into from

Conversation

evanchaoli
Copy link
Contributor

No description provided.

Signed-off-by: Chao Li <[email protected]>
@evanchaoli evanchaoli changed the title WIP: Step "gate" Step "gate" Jul 29, 2020
Signed-off-by: Chao Li <[email protected]>
Copy link
Member

@vito vito left a comment

Choose a reason for hiding this comment

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

👋 Finally getting around to this, thanks for submitting it! This touches on a lot of interesting ideas that have come up before in different ways, but I think there are a lot of tradeoffs to discuss.

- gate: code-coverage-gate
condition: ((.:scan-result.coverage_rate)) < 0.85

```
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}

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.

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)

- gate: no-release-gate
condition: no-release in ((.:mr_info.labels))
fail: false
```
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.

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.


## 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.

@holgerstolzenberg
Copy link

holgerstolzenberg commented Oct 12, 2021

What is the current status of this RFC. Has it already landed within Concourse without being documented.

I have a clear use case for it by now:

Assume having a monorepo for Angular apps. Having all the code for all apps in one place means we need tooling to determine what to build. We achieve this by using nx (https://nx.dev/). We managed to use the across step to setup some pretty cool working pipeline, but I could not manage to dynamically push the related ui docker images, as Concourse does not support that by design (concourse/concourse#7560).

Of course we can work around by something like (pseudo):

- in_parallel:
  - try: "determine-app1-image-was-build-task"
    on_succes:
        put: "app1-image"

but this is a bit cumbersome and always means we have to fail "determine-app1-image-was-build-task" task for not doing the image put.

Having the conditional would be great:

- in_parallel:
  - put: "app1-image"
    condition: "master-build/dist/app1/image.tar exists"
  - put: "app2-image"
    condition: "master-build/dist/app2/image.tar exists"

Do not nail me on how the condition could actually look like, but with all the options discussed above, i think it is doable and would ease up a lot of things within Concourse.

Side note: The across step should definitely make it into Concourse.

@suhlig
Copy link

suhlig commented Jan 26, 2022

In general, I am not convinced that a gate step is needed.

  1. Adding more building blocks to Concourse means people need to understand even more things. I would prefer fewer, but more powerful concepts.
  2. I would expect it to be very hard to come up with a reasonably universal way to express the condition. For all but the most trivial cases I prefer a programming language where I can write the code in isolation and could even add tests. With this in mind, a gate would not be that much different from a task.

@holgerstolzenberg
Copy link

holgerstolzenberg commented Feb 2, 2022

Well okay I would agree that finding a simple yet powerful expression language for evaluating the boolean condition might be hard. Possibly there is already some library out there that might fit.

Second - the step would still be very useful. As I said we have a lot of use cases in our pipes, especially when it comes to monorepo stuff that in fact is a new reality for frontend teams.

A compromise could be: Just have a gate step that evaluates upon a boolean. The boolean has been made available in the pipeline before using e.g. load_var step. How you create the boolean is up to you (param, static constant, created by task).
This way, the gate step would be fairly "simple" and Concourse team would not have to worry about inventing an expression language.

@JohannesRudolph
Copy link

I've built https://github.com/meshcloud/gate-resource a long while back and it happily serves our production pipelines. Alas, gates can be built as resources. The problem here is that like lots of things concourse - there's great fundamental abstractions and extension point but assembling any non-trivial behavior out of them (like conditional pipeline flows) is quite hard and convoluted. Analogy: Feels like using git without the porcelain around it.

Adding a "gate" as a first class principle to concourse would be a great opportunity to enhance the user experience for a common use case and make it a bit easier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants