rfc: generic per-route features #743
Conversation
7d449a6 to
99ccb12
Compare
|
ping @geofffranks @ameowlia |
8aec539 to
0ce0b58
Compare
|
@maxmoehl I see that this RFC is not marked as draft any more. I assume that this is ready for review and add it to the list of RFCs which the TOC will look into next Tuesday. If it still not the case please let me know. |
| - name: test | ||
| routes: | ||
| - route: example.com | ||
| options: |
There was a problem hiding this comment.
If the route already exists without these options would you expect it to be updated?
There hasn't really been much updating of routes before so you might want to mention that this endpoint would be enhanced:
https://v3-apidocs.cloudfoundry.org/version/3.159.0/index.html#update-a-route
And probably worth mentioning https://v3-apidocs.cloudfoundry.org/version/3.159.0/index.html#create-a-route as well. cf create-route and cf map-route (if the route doesn't already exist) both call that one.
There was a problem hiding this comment.
Good point! Yes, we expect those options to be modifiable via updates.
There was a problem hiding this comment.
I'm not that familiar with the CC API, are such PATCH calls usually additive or do they replace the object?
There was a problem hiding this comment.
For the route as a whole it would be additive. You wouldn't need to specify fields like host or path again.
The CC API isn't as consistent for object subfields though. I think you could either replace the whole options field with the contents of the PATCH or do something like the update env vars endpoint and make null a valid value for removing an option. I think replacing it is probably simpler and I think the default behavior in CCNG.
There was a problem hiding this comment.
Do you think this should be included in the RFC? I'd probably leave such details up to the implementers.
I've also added the relevant API endpoints to the RFC.
There was a problem hiding this comment.
I think this (indeed surprising) behaviour is correct according to the API spec: https://v3-apidocs.cloudfoundry.org/#apply-a-manifest-to-a-space
Apply changes specified in a manifest to the named apps and their underlying processes. The apps must reside in the space. These changes are additive and will not modify any unspecified properties or remove any existing environment variables, routes, or services.
Since the protocol property was not specified, it remains unchanged which results in http2. The diff output could be considered as buggy.
Disclaimer: I don't understand the coding in every detail and I could not find a matching test case that deals with this protocol case. I will raise this in the CAPI Open Office Hour.
There was a problem hiding this comment.
We discussed this during the TOC meeting today and we should go for consistent behaviour as documented in the APIs.
There was a problem hiding this comment.
I've added a paragraph to capture that:
The overall goal MUST be to implement the new fields in a way that fits in with the current behavior of the cloud controller API. It is up to the maintainers / implementers to decide on the exact behavior.
There was a problem hiding this comment.
I don't have a terribly strong opinion here, but I think we should go with what's consistent for now and provide a way for users to explicitly unset the properties in the manifest.
I know that @Gerg has been thinking about what a truly declarative "version 2" of the CF manifest my look/behave like and I think that would be an opportunity to incorporate some of that feedback. He has a doc somewhere, but I think it was made private due to some Google doc migrations. I'll let him chime in when he's available / let him know about this.
There was a problem hiding this comment.
we should go for consistent behaviour as documented in the APIs.
I agree. I also remember being confused about this behavior regarding readiness health check manifest properties. It's odd, but that is how the app manifest currently works. I think changing it is out of scope for this RFC.
|
I've been resolving conversations where I either adjusted the RFC or think that we have consensus on not addressing the comment in this RFC, the only discussion left right now is about API semantics in CC. If you had comments please double check that they are addressed appropriately and unresolve discussions which still need clarification! I would like to move into the final comment period soon-ish. |
|
We decided to start the FCP for this RFC with the goal to accept it next Tuesday 16th of April. |
🚀 Link for easy viewing.
/cc @domdom82 @peanball