-
Notifications
You must be signed in to change notification settings - Fork 702
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
Add Kubeapps apis docs #2885
Add Kubeapps apis docs #2885
Conversation
commit: 2e73676eef8642dfba4ed782b7c8d6fe | ||
digest: b1-vB11w98W2vFtEP4Veknm56Pi6DU6MpOuocESiOzvbqw= | ||
create_time: 2021-04-26T14:55:30.644663Z |
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 wasn't able to replicate your issue, @absoludity . I did type buf beta mod updata
and then buf generate
without any error :S
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.
Excellent, I'll recheck locally once this lands and update the issue I created.
# https://github.com/grpc-ecosystem/grpc-gateway/pull/2147 | ||
- name: openapiv2 | ||
out: docs | ||
strategy: all |
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 was the key point for allow_merge
to work...
For the record, the default value is directory
, so the merge logic, in each folder, was overwriting the file again and again.
Anyway, it was worthwhile as I have learned a bit more about buf :P
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.
Nice - so we can combine/merge the schema docs without needing the (npm-based) swagger-combine? Excellent if so!
# - generate_unbound_methods=true # have to re-check it | ||
# - json_names_for_fields=false # have to re-check it |
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 have to see the impact of these options. I have screened the ones that other people are using in other github projects, but I have to re-check when playing around with the generate API docs
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.
Excellent, very happy to see a single doc without swagger-combine :) +1'ing now just in case you only need minor changes and I'm not around.
# https://github.com/grpc-ecosystem/grpc-gateway/pull/2147 | ||
- name: openapiv2 | ||
out: docs | ||
strategy: all |
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.
Nice - so we can combine/merge the schema docs without needing the (npm-based) swagger-combine? Excellent if so!
commit: 2e73676eef8642dfba4ed782b7c8d6fe | ||
digest: b1-vB11w98W2vFtEP4Veknm56Pi6DU6MpOuocESiOzvbqw= | ||
create_time: 2021-04-26T14:55:30.644663Z |
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.
Excellent, I'll recheck locally once this lands and update the issue I created.
{ | ||
"swagger": "2.0", | ||
"info": { | ||
"title": "kubeappsapis/core/packages/v1alpha1/packages.proto", |
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 like the info defines this as core.packages.v1alpha1 only, whereas its describing others too (like core.plugins.v1alpha1
and plugins.fluxv2.packages.v1alpha1
etc.)?
Conflicts: cmd/kubeapps-apis/proto/kubeappsapis/core/plugins/v1alpha1/plugins.proto
@@ -4,15 +4,30 @@ option go_package = "github.com/kubeapps/kubeapps/cmd/kubeapps-apis/gen/plugins/ | |||
|
|||
import "google/api/annotations.proto"; | |||
import "kubeappsapis/core/packages/v1alpha1/packages.proto"; | |||
import "protoc-gen-openapiv2/options/annotations.proto"; | |||
|
|||
service PackagesService { |
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.
Question here, @absoludity : is it possible to safely change this service name to something like kappController_PackagesService
?
The generator is not able to deal with duplicated service names and we have to manually disambiguate the operation_id
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.
Yes, I think KappControllerPackagesService should be fine. The only restriction buf lint
puts on is that it needs to end in Service
, from memory. Feel free to update to that (and Fluxv2PackagesService) and see if that fixes it before landing. Check buf lint too.
Initial version done! I have a minor question (as a comment), just to avoid using things like this to disambiguate names. option (grpc.gateway.protoc_gen_openapiv2.options.openapiv2_operation) = {
operation_id: "xxxxxxx";
... Apart from that, I've tried to stick to the comments as much as possible (instead of using // Title of the message
//
// Description of the message
message xxxxxxxxxx {
option (grpc.gateway.protoc_gen_openapiv2.options.openapiv2_schema) = {
example: '{ a json example of the object }'
};
// Title of the message field (except if it is a XXXXRequest, since they have no title, just description)
//
// Description of the message field
XXXX xxx = 1;
} |
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.
Great!
@@ -4,15 +4,30 @@ option go_package = "github.com/kubeapps/kubeapps/cmd/kubeapps-apis/gen/plugins/ | |||
|
|||
import "google/api/annotations.proto"; | |||
import "kubeappsapis/core/packages/v1alpha1/packages.proto"; | |||
import "protoc-gen-openapiv2/options/annotations.proto"; | |||
|
|||
service PackagesService { |
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.
Yes, I think KappControllerPackagesService should be fine. The only restriction buf lint
puts on is that it needs to end in Service
, from memory. Feel free to update to that (and Fluxv2PackagesService) and see if that fixes it before landing. Check buf lint too.
Sorry, probably conflicts (and two extra doc comments) to update when I landed my PR. |
Conflicts: cmd/kubeapps-apis/gen/core/packages/v1alpha1/packages.pb.go cmd/kubeapps-apis/proto/kubeappsapis/core/packages/v1alpha1/packages.proto
PR in progress, just for showing the current changes.
Description of the change
We will have a way to automatically generate our API docs. After importing the OpenAPI file in any tool we will get a ready-to-use client, for instance:
Benefits
We will have automatic API docs!
Possible drawbacks
N/A, but once integrated as "official" docs, we should think about how to integrate it with the existing Kubeapps API docs
Applicable issues
Additional information
~~I still need to work on defining the proper annotations, like: ~~ DONE
An example of how to use the OAS document to easily consume the API :