-
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
RFC: Resources v2 #1
Conversation
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'm looking forward to building against this. I haven't totally wrapped my head around spaces and I really liked the original proposal as it was, so I hope we can get this back to that same level of confidence and clarity.
One thing that i'd like to put out there explicitly is that even the tiniest interface ergonomics improvements can make a huge difference to first time resource implementers. Many small things like not being able to log to stdout, having different command line parameters for each binary, in==get, out==put, understanding multiline yaml scalar => json conversions, multiple sources of configuration, etc, etc can make the current interface feel like death by a thousand cuts when you're first wrapping your head around it. The original proposal addressed those sorts of issues more directly, and though this one still does I hope we don't lose sight of the small things.
01-resources-v2/proposal.md
Outdated
|
||
It may be the case that most resources cannot easily support `destroy`. One | ||
example is the `git` resource. It doesn't really make sense to `destroy` a | ||
commit. Even if it did (`push -f`?), it's a kind of weird workflow to support |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
01-resources-v2/commit-status.yml
Outdated
plan: | ||
- get: atc-pr | ||
trigger: true | ||
spaces: all |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
* removed the `destroy` action. instead, `put` returns a set of versions that were deleted, alongside the set of versions that were created. * removed the `spaces` action. instead, `check` now runs in batch across all spaces. * `check` now returns *all versions*, both on the initial call and if the given versions are no longer present (push -f). this will be a problem for resources with lots of history - maybe either streaming or pagination would help in the future? * space identifiers are back to normal strings, rather than JSON objects. i'm not 100% convinced of this but it definitely makes it easier to use them as keys in maps, and should be cleaner in the UI and in YAML.
space is a string, not a JSON object
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.
Looks really good in my opinion! My Ruby is not so strong, but most of what I saw was as expected from the proposal - except for the check
request which I also left a comment about. I also left a "renaming" comment in there for your consideration. Might be too wild an idea, but I left it there anyhow ;)
Other than that I just have two general things:
- Webhook artifact is on my wishlist. Not sure how hard it would be to forward the webhook payload to a resource, but I really think it is something that could help us build next level resources.
- I'm not sure why partial implementations (understood as not implementing all interfaces) for resources is considered a bad thing? Added an issue around this question here.
I'm going to cut the scope of this RFC down a bit by removing all the stuff concerned with notifications. I think there's a lot more to explore there and it warrants its own RFC. I'd rather wrap up the new artifacts interface sooner so that we can get support for spaces out the door. I also just realized we haven't at all discussed the idea of schema validation. I'm going to cut that out, too - we should be able to add that in later, again as a separate RFC. |
also update examples to use simple string space identifiers, and clarify some of the change summaries
didn't get time to map this out - let's do it as a separate RFC
also switch git example to rugged, for much more efficient checking across branches (no need to checkout tree, no need to shell out to git)
|
||
If the requested version is unavailable, the command should exit nonzero. | ||
|
||
No response is expected. |
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.
After discussing with @clarafu we think it'd actually be better if get
s still returned a response, and have that response include metadata.
This makes get
, check
, and put
all have a response, and lets the resource author decide where metadata should be discovered. Now the difference between v1 and v2 would be that with v2 check
can also return metadata - an additive change, rather than requiring all metadata come from check
.
This way if there are situations where it's too expensive to collect all the metadata at once, it can be deferred until later get
and put
calls. Any metadata returned by get
and put
would just update the version's metadata.
This also makes the implementation in the ATC a bit cleaner because we don't have to worry about a scenario where e.g. a get
is running with a version that has never had a check
run and so there's no metadata to show to the user in the UI. This normally won't happen but is theoretically possible via the builds API, where you can submit a build that fetches an arbitrary version. This also makes v1 and v2 more similar to each other, minimizing conditional code paths in the implementation.
"resources" concept, but with a more specific name. | ||
|
||
* There are no more hardcoded paths (`/opt/resource/X`) - instead there's the | ||
single `info` entrypoint, which is run in the container's working directory. |
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.
info
semantically implies read-only. If it is going to replace also the out
/put
, maybe a more adequate name could be command
or similar.
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.
Confusing wording - it's read-only, I meant entrypoint
from a discovery standpoint (invoke info
to learn the later commands to run), not an execution standpoint. Will amend.
rather than taking the path as an argument. This was something people would | ||
trip up on when implementing a resource. | ||
|
||
* Change `check` and `put` to write its JSON response to a specified file, |
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.
its -> their
response -> responses
trip up on when implementing a resource. | ||
|
||
* Change `check` and `put` to write its JSON response to a specified file, | ||
rather than `stdout`, so that we don't have to be attached to process its |
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.
so that we don't have to be attached
this is unclear. Who is attached to what ?
A question I've thought of whether resources could add a Arguing against: any The inspiration for this thought bubble is watching over the shoulder of Knative Eventing. In Concourse terms, I think Knative Eventing somewhat mixes together the concerns of provisioning an outside service with configuration of |
I've been doing a lot of thinking about this lately, and now I'm worried that baking a high-level concept like 'spaces' into the resource interface is the wrong direction. I think I've got a better idea that simplifies things down to a single resource interface, a lot like what we have today - and a plan to use this single interface to implement spaces, triggers (e.g. the There's a lot to go over here - I'm going to try and provide as much context as possible behind my thought process. I expect at least some of this to be confusing or unclear. Sorry in advance for the word vomit. 😅 Feel free to ask questions to clarify - I plan to open a new RFC as it's a significant enough departure from this proposal, so we might as well just open the discussion up here until then. Triggers and the thundering herd problemWith concourse/concourse#2386 we formalized how resource configs and their versions are stored abstractly, but this introduced a problem: now the Resource scopes proved challenging to implement and introduce a level of overhead that is only exacerbated by the addition of resource spaces to the model, as they both try to achieve similar things. This led us to try to rethink the But then I thought maybe we're over-complicating things by introducing a whole new interface. If you look at the Coincidentally, @chenbh opened concourse/concourse#3572 around the same time we were thinking about this. What's interesting is here they still want to use a resource, but only as a trigger, and they don't want to fetch the bits. Sounds familiar! 🤔 That led me to think maybe the interface is fine, and we just need different ways to use resources in a pipeline. Maybe everything is just "resources" in the sense that they're all the inputs to your pipeline and still represent all external state, but maybe we just need to allow your pipeline to have different relationships with this state? Maybe everything just isn't versioned artifacts? It's worth noting briefly that the thundering herd problem still exists within a pipeline today, not just with globally shared history. Having all the jobs that point to a time trigger all fire at once doesn't seem great. So what if you could associate a resource to a job, and have its Spatial resourcesIf we go with v2 RFC as-is, all resources implicitly have to care about spaces. What does this 'trigger-only' workflow even mean for spaces? ...Should spaces just be optional? What if spaces wasn't part of the interface at all? Just like we were able to implement triggers in terms of the resource interface, can we implement spaces in terms of it, too? The meaning of
What's nice is this makes the detecting of spaces a lot clearer, and supports explicit creation/deletion of the spaces themselves. The current proposal puts all of this into So, how would we use this to support spaces over an artifact resource like We take the "versions" returned by the space
In this case, the The interesting thing here is it uses the 'version' as a way to compose resources together. It actually feels pretty nice! Versions are already a public contract and are even shown to users directly in the UI. What's really interesting about this is that by decoupling the space resource from the artifact resource, you could space over arbitrary things. At the end of the day you're just dynamically providing fields to This also removes the whole idea of a "default space". Now that resources don't have to implement spaces, it's not necessary to even think about as a resource type author. Either your Another thing that's great about this is that, internally, everything is just back to resource configs and their versions. No more scopes, spaces, etc. - just resource configs and their associated versions. In this relationship, one resource config just results in another resource config being created. NotificationsAn early iteration of this RFC actually introduced notifications as a new part of the v2 interface, and they could be tied to artifact resources. This was so that things like GitHub status checks could be sent back with an associated version, so the notification resource knows what commit to apply the status check to. So, what if we don't add a new interface for notifications, and we just use the 'version' as a communication mechanism between resources, just like we did for spaces? What if you could point a notification resource at an artifact resource, and have Concourse automatically run the notification resource when the artifact resource is used in a build? This would work by taking the version of the artifact resource and tossing it in the {
"config": {
# arbitrary user-specified config
"repo": "concourse/concourse",
# taken from the monitored resource
"ref": "abcdef"
},
# provided by concourse
"build": {"id": 123, "status": "failed", "job_name": "some-job", "url": "https://..."}
} Overall proposalI think we should go back to resources being a super simple and general The new proposal would introduce the general interface, and then describe how it would be interpreted to support 'versioned artifacts', as the interface is used for today, and also touch on the ideas around composability I covered above, ultimately so that the common interface isn't too tied to versioned artifacts. (For example, we may want to use a name other than 'version'.) The challenging part of this approach is how we bubble these concepts up to users. The pipeline has to feel intuitive and not too repetitive. It shouldn't take a degree in YAML architecture to configure GitHub status checks. But the more I think about this, the more apparent the benefits become in terms of the mental model and internal structure. I think having a consistent model is paramount for Concourse's success (it's one of the founding principles). Here are some key take-aways summarizing the general direction I think we should go:
Critically, those last 3 RFCs can be done with both the v1 and v2 interfaces. It's not so coupled anymore. If you subtract spaces from resources v2, a lot of it is boring protocol changes, plus things like " |
Will the lengthy comment above somehow come across to the new proposal? It carries a lot of important context and food for thought. |
@jchesterpivotal The new proposals will encompass everything covered there, yes, though at the moment in a bit of a dry sense - the RFCs just outline the 'what' whereas my comment earlier was kind of a brain dump/train of thought that led to it. Is there any particular part you feel I should make sure to incorporate? Maybe they could just reference my comment for context-building? 🤔 |
Perhaps as a discussion document? I'm concerned that lots of folks will overlook a detailed discussion on a closed PR. |
@jchesterpivotal I've added a 'Previous Discussions' section to the new RFC: 0a805ec |
Closing in favor of #24. I was gonna wait on the post announcing it per the RFC resolution process but then realized folks can just close their own proposals without having to go through the resolution process. 😅 I'm writing a post for #24 anyway though and will mention the closing of this one. |
Rendered
Status: ~80% figured out. The remaining 20% is concerning performance/architectural implications that we should be sure to understand before going forward.
This is a very large proposal, heavily influenced by concourse/concourse#1707 and with a lot of prior planning in concourse/concourse#534. The discussion in 534 was growing stale, which was the catalyst for this new RFC format. Please leave feedback on individual lines instead of commenting directly so that the number of open discussion threads dwindles down as the proposal stabilizes and improves.
TODOs: