Skip to content

support Deprecated field #275

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

support Deprecated field #275

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
37a01db
Upgrade to restful v2.9.5 and support Deprecated field
Jefftree Jan 18, 2022
0c38d6d
Rebase indent
Jefftree Aug 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions pkg/builder/openapi.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,7 @@ func (o *openAPI) buildOperations(route common.Route, inPathCommonParamsMap map[
Description: route.Description(),
Consumes: route.Consumes(),
Produces: route.Produces(),
Deprecated: route.Deprecated(),
Schemes: o.config.ProtocolList,
Responses: &spec.Responses{
ResponsesProps: spec.ResponsesProps{
Expand Down
1 change: 1 addition & 0 deletions pkg/builder3/openapi.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ func (o *openAPI) buildOperations(route common.Route, inPathCommonParamsMap map[
ret := &spec3.Operation{
Copy link
Member

@liggitt liggitt Jan 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

for openapi v3, is it possible to set the deprecated field on the schema object as well as the operation?

Copy link
Member

@apelisse apelisse Jan 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, that'd be super sweet, especially if we automatically enable new mechanisms to help with deprecation.

Copy link
Member Author

@Jefftree Jefftree Jan 19, 2022

Choose a reason for hiding this comment

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

Yeah it should possible but it's not as straightforward and I thought it might be better to include in a follow up after some more discussion.

I'm still doing some investigations, but afaik we can't guarantee that a deprecated operation path would deprecate all the corresponding schemas associated with that path. An example of this is a delete on an alpha/beta resource that returns a meta.v1.Status. Is this a one-off exception or are there more examples of this?

For schema fields, there are cases where we deprecate a single schema field and not the entire schema. This is currently embedded in field descriptions, and it may be beneficial to include these programmatically as well.

Copy link
Member

@apelisse apelisse Jan 19, 2022

Choose a reason for hiding this comment

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

afaik we can't guarantee that a deprecated operation path would deprecate all the corresponding schemas associated with that path

IIUC, it shouldn't. Because an operation is deprecated doesn't mean that the type that it received is also deprecated.

Thanks for the perspective, I agree that it'd be nice to have deprecated either for entire types (v1beta1 type is deprecated) or for fields, but we would probably need new markers and what not, probably out of the scope of this project (but the types of things that are enabled by this project).

Copy link
Member

@liggitt liggitt Jan 19, 2022

Choose a reason for hiding this comment

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

If you look at how this deprecated operation setting is being driven from k/k, it's based on the entire primary struct for a set of operations being deprecated. Even if it's in another PR, having a way to plumb the fact that that struct is deprecated into openapi would be useful.

Copy link
Member

@apelisse apelisse Jan 19, 2022

Choose a reason for hiding this comment

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

OK, thanks for clarifying, I completely agree.

OperationProps: spec3.OperationProps{
Description: route.Description(),
Deprecated: route.Deprecated(),
Responses: &spec3.Responses{
ResponsesProps: spec3.ResponsesProps{
StatusCodeResponses: make(map[int]*spec3.Response),
Expand Down
2 changes: 2 additions & 0 deletions pkg/common/interfaces.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ type Route interface {
// StatusCodeResponses defines a mapping of HTTP Status Codes to the specific response(s).
// Multiple responses with the same HTTP Status Code are acceptable.
StatusCodeResponses() []StatusCodeResponse
// Deprecated marks a route as deprecated
Deprecated() bool
Comment on lines +39 to +40
Copy link
Contributor

@austince austince May 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Will this be released as a breaking change? iirc, we were quite careful when adding this interface to not break backwards compatibility, suggest doing the same here. ref: #252 (comment)

Copy link
Member Author

@Jefftree Jefftree Aug 9, 2022

Choose a reason for hiding this comment

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

I did a quick scan of the k/k codebase, it looks like nothing is using the particular interface and all calls of the interface are from kube-openapi itself. The GetOperationIDAndTagsFromRoute in the PR linked has not yet been integrated into k/k yet.

So to answer your question, yes this will be a breaking change for current users of the API, but at the moment it seems like we have no users of the particular API (that we know of). I'm okay with taking the safe approach of just creating another interface, just want to gather some thoughts first.

Copy link
Contributor

@austince austince Aug 10, 2022

Choose a reason for hiding this comment

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

Thanks for your reply. I agree it isn't a huge breaking change and won't affect k/k, though will affect other projects using it (mine 😄), perhaps it's acceptable for this project to only support k/k.
Do you think it would be valuable to switch k/k to the interface, or rather wait until there's a push to get off go-restful?

Copy link
Member Author

@Jefftree Jefftree Aug 10, 2022

Choose a reason for hiding this comment

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

Thanks for pointing that out, that's enough of a reason to create backwards compatibility and create a new interface. Do you have any preferences for the interface name?

I don't seem any harm in switching k/k to the interface, although that will be a decision up to the approvers.

Copy link
Contributor

@austince austince Aug 10, 2022

Choose a reason for hiding this comment

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

Thanks very much for the backwards compat effort :) No real preference on name, perhaps DeprecatableRoute?

}

// StatusCodeResponse is an explicit response type with an HTTP Status Code.
Expand Down
4 changes: 4 additions & 0 deletions pkg/common/restfuladapter/route_adapter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,3 +66,7 @@ func (r *RouteAdapter) RequestPayloadSample() interface{} {
func (r *RouteAdapter) ResponsePayloadSample() interface{} {
return r.Route.WriteSample
}

func (r *RouteAdapter) Deprecated() bool {
return r.Route.Deprecated
}