-
Notifications
You must be signed in to change notification settings - Fork 35
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
base: master
Are you sure you want to change the base?
Step "gate" #66
Conversation
Signed-off-by: Chao Li <[email protected]>
Signed-off-by: Chao Li <[email protected]>
There was a problem hiding this 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 | ||
|
||
``` |
There was a problem hiding this comment.
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}
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 | ||
``` |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
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 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 |
In general, I am not convinced that a
|
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 |
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. |
No description provided.