-
Notifications
You must be signed in to change notification settings - Fork 102
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
Refactors API Service for API versioning #174
Conversation
/hold |
d6b22a2
to
f2a54cc
Compare
f2a54cc
to
68f1ac2
Compare
|
||
# For each new version, doc has to be copied | ||
COPY gen/http/openapi3.yaml /app/gen/http/openapi3.yaml | ||
COPY v1/gen/http/openapi3.yaml /app/v1/gen/http/openapi3.yaml |
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 according to api policy will we make change here if further we have v2 and so on or keep this as well ??
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.
for each new version, we need to copy the doc.. if we are deprecating a version then we will remove that doc
api/pkg/shared/resource/resource.go
Outdated
NotFoundError = fmt.Errorf("resource not found") | ||
) | ||
|
||
//Query resources based on name, kind, tags. |
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.
nit: Can we please add a space after //
Description("The resource service provides details about all kind of resources") | ||
|
||
HTTP(func() { | ||
Path("/v1") |
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.
Just a thought is it possible to define the path somewhere so that we won't have to write the whole code again instead just take the path and rest all api design take it from generalized directory (this implies mostly when api's are not removed)
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.
define the path somewhere
where? 😄
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.
What I meant was the same thing we have done by creating a shared
directory, not sure will that work or not just asking
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 think for the design we can't do that we define the URL inside the service even if we share a service can't override any values
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.
this is fine I guess easy to maintain
68f1ac2
to
62766e2
Compare
api/design/category.go
Outdated
}) | ||
|
||
HTTP(func() { | ||
GET("/categories") | ||
GET("/v1/categories") |
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.
This will be breaking change now as we already published
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.
added back.. API will be removed in the next release and be internal to hub
62766e2
to
85e92d0
Compare
This refactors API service code to maintain seperate directory for each user facing APIs. Signed-off-by: Shivam Mukhade <[email protected]>
85e92d0
to
2bb4750
Compare
/lgtm Later, we may work on generalizing the API layer also like DB layer between multiple version services |
/lgtm |
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: PuneetPunamiya The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/woof |
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Signed-off-by: Shivam Mukhade [email protected]
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide for more details.