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

Configuration & Route lifecycle operations are required #36

Closed
wants to merge 1 commit into from

Conversation

dprotaso
Copy link
Member

@dprotaso dprotaso commented Aug 9, 2021

This allows for portability of these resources for advanced users
who do not use Knative Service

This allows for portability of these resources for advanced users
who do not use Knative Service
@google-cla google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Aug 9, 2021
@knative-prow-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dprotaso
To complete the pull request process, please assign duglin after the PR has been reviewed.
You can assign the PR to them by writing /assign @duglin in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@knative-prow-robot knative-prow-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Aug 9, 2021
@eallred-google
Copy link
Contributor

I don't think we should change this too required. Can you provide more rationale regarding making this change? Portability could arguably be applied to every API setting and call.

@salaboy
Copy link
Member

salaboy commented Aug 10, 2021

I do agree with @dprotaso in the sense that Knative clients like the CLI tools are already using those APIs.

@dprotaso
Copy link
Member Author

dprotaso commented Aug 10, 2021

Can you provide more rationale regarding making this change?

Configuration & Route were never marked internal so integrators always had the option to drop to these APIs for flexibility. There's already an example of this where projectriff (now archived) was using configuration and route directly. I believe this was done to get around some issues with revision management.

A use case I'm aware of is the need for stable routing over configurations instead of revisions

Copy link
Member

@evankanderson evankanderson left a comment

Choose a reason for hiding this comment

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

I think it's okay for some clients (and maybe even the OSS CLI) to use operations which aren't marked as REQUIRED in the spec.

For a specific example, I think this may break Cloud Run (Fully Managed), because I'm not sure that they have support for free-range Configurations without an associated Service. @whaught to confirm here, since I honestly don't remember.

Comment on lines -365 to -369
The table below details which operations must be made available to a developer
accessing a Knative Route using a minimal profile. For any non-minimal profile,
the POST, PUT, or DELETE operations MUST be enabled as a group. This ensures
that the developer has the ability to control the complete lifecycle of the
object from create through deletion.
Copy link
Member

Choose a reason for hiding this comment

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

Why remove this verbiage?

Copy link
Member Author

Choose a reason for hiding this comment

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

POST, PUT, or DELETE operations were marked as REQUIRED in tandem. The only remaining OPTIONAL operation is PATCH so it didn't seem necessary to keep this clause

@whaught
Copy link
Contributor

whaught commented Aug 16, 2021

I think it's okay for some clients (and maybe even the OSS CLI) to use operations which aren't marked as REQUIRED in the spec.

For a specific example, I think this may break Cloud Run (Fully Managed), because I'm not sure that they have support for free-range Configurations without an associated Service. @whaught to confirm here, since I honestly don't remember.

This would likely break Cloud Run. Internally we do support actions on Configurations and Routes and it's likely they could be made public, but doing so is out of scope for the simple experience we are intending to provide. Ideally this would not be required.

@dprotaso
Copy link
Member Author

dprotaso commented Aug 17, 2021

but doing so is out of scope for the simple experience we are intending to provide. Ideally this would not be required.

This would offer integrators, who might need to drop to lower levels, portability

@yanweiguo
Copy link
Contributor

yanweiguo commented Aug 19, 2021

From knative/serving#412, I think the de-composed resource model wasn't compatible with our ability to deliver a great developer experience so we introduced Service. IMO we should encourage developers to use service directly. Marking Configuration & Route create/update API as REQUIRED sounds an opposite to simple experience to me.

@dprotaso
Copy link
Member Author

From knative/serving#412, I think the de-composed resource model wasn't compatible with our ability to deliver a great developer experience so we introduced Service

I agree that Service is the abstraction that developers should use

Marking Configuration & Route create/update API as REQUIRED sounds an opposite to simple experience to me.

I'm suggesting these resources are for 'integrators' of Knative who understand the extra complexity in exchange for flexibility

@xtreme-sameer-vohra
Copy link

Serving overview has an interesting description thats relevant here

Service acts only as an orchestrator of the underlying Route and Configuration (much as a kubernetes Deployment orchestrates ReplicaSets). Its usage is optional but recommended.

@whaught
Copy link
Contributor

whaught commented Aug 25, 2021

Right. Integrators should be fine to use Route and Configuration directly in their implementation of the spec if they choose. However we also think offerings like Cloud Run which aim to offer a simplified experience should also be allowed to lock down the experience to one single endpoint (Service) without it being required that we allow further complexity from clients. Either should still be considered in compliance with Knative which allows for diversity in the experiences vendors wish to offer.

This difference ought not be required of any vendor lest a particular implementation become the de-facto specification.

@duglin
Copy link
Contributor

duglin commented Aug 26, 2021

/LGTM

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 26, 2021
@xtreme-sameer-vohra
Copy link

Serving overview has an interesting description thats relevant here

Service acts only as an orchestrator of the underlying Route and Configuration (much as a kubernetes Deployment orchestrates ReplicaSets). Its usage is optional but recommended.

To clarify, I understood ☝️ that as Service's usage is optional but recommended and therefore Route and Configuration being required.

@bsnchan
Copy link

bsnchan commented Aug 26, 2021

/LGTM

@dprotaso
Copy link
Member Author

This was veto'd by Google (@spencerdillard)

@dprotaso dprotaso closed this Sep 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
Development

Successfully merging this pull request may close these issues.

10 participants