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.

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

Jump to bottom

Kubectl: Replace usages of swagger with open API #44589

Closed
pwittrock opened this issue Apr 17, 2017 · 10 comments · Fixed by #53232
Closed

Kubectl: Replace usages of swagger with open API #44589

pwittrock opened this issue Apr 17, 2017 · 10 comments · Fixed by #53232
Assignees
@mbohlool @mengqiy
Labels
area/kubectl sig/cli Categorizes an issue or PR as relevant to SIG CLI.

Comments

@pwittrock
Copy link
Member

Deprecate the discovery API returning Swagger and replace all instances with open API
@mengqiy
Copy link
Member

mengqiy commented Apr 19, 2017

@mbohlool We should come up with a TODO list of replacing usages of swagger with openAPI.

@mbohlool
Copy link
Contributor

@mengqiy sounds great. I am not familiar with kubectl code. Can you list all usage of old swagger in kubectl here? A link to packages would be enough but a link to actual usages (or samples of usages in those packages) would be much better. We can then create a list of tasks for it.

@earth2marsh
Copy link

If you need guidance on how to use the name and/or logo of the OpenAPI Specification, there's a style guide here: https://github.com/OAI/OpenAPI-Style-Guide#the-specification-name-and-usage

@mengqiy
Copy link
Member

mengqiy commented May 4, 2017 •
edited
Loading

@mbohlool Sorry for the delay.

The usage of old swagger:
kubectl retrieve swagger using discovery API.
It is implemented here.
And swagger is being used for validation(Validator)

And there are several commands call the Validator() function to do the validation. If you need them, I will list them.

@mbohlool
Copy link
Contributor

mbohlool commented May 4, 2017

Thanks. From what I read, we have two options:

  1. Read openapi spec instead of swagger and convert it inline to old swagger (so no change in validation code)
  2. Fix validation code to use openapi instead of swagger (prefered). You can use OpenAPI loader @pwittrock wrote.
    Doing first one seems to be faster, but keep in mind, you need to cache result because it taking some time to load. I really prefer second option.

@mengqiy
Copy link
Member

mengqiy commented May 4, 2017

I'm leaning to the second option, too.
But as discussed in #39434 the client side validation will be moved to the server-side in 1 or 2 releases. So the client-side validation code won't live long.

@mbohlool
Copy link
Contributor

mbohlool commented May 4, 2017

That means we should support old swagger for 2-3 releases. How much work do you think option 2 is?

@mengqiy
Copy link
Member

mengqiy commented May 4, 2017

That means we should support old swagger for 2-3 releases.

That's not good. We definitely should use OpenAPI.

How much work do you think option 2 is?

Should not be too much.

@mengqiy mengqiy mentioned this issue May 9, 2017
Closed
@0xmichalis 0xmichalis added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label May 21, 2017
@mbohlool mbohlool mentioned this issue Jun 2, 2017
Closed
3 tasks
@apelisse apelisse mentioned this issue Jul 27, 2017
Merged
k8s-github-robot pushed a commit that referenced this issue Jul 28, 2017
Merge pull request #49146 from apelisse/openapi-new-structure
bc3c5bc 
Automatic merge from submit-queue (batch tested with PRs 49665, 49689, 49495, 49146, 48934)

openapi: refactor into more generic structure

**What this PR does / why we need it**:
Refactor the openapi schema to be a more generic structure that can be
"visited" to get more specific types. Will be used by validation.

**Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: #44589

**Special notes for your reviewer**:

**Release note**:
```release-note
NONE
```
@bgrant0607
Copy link
Member

dupe of #38637?

@mbohlool
Copy link
Contributor

mbohlool commented Sep 7, 2017 •
edited
Loading

I think this is more about kubectl, @pwittrock do you agree? if that is the case then title should reflect that.

@mbohlool mbohlool mentioned this issue Oct 2, 2017
Closed
@apelisse apelisse mentioned this issue Oct 3, 2017
Merged
@apelisse apelisse changed the title Replace usages of swagger with open API Kubectl: Replace usages of swagger with open API Oct 4, 2017
@apelisse apelisse added area/kubectl sig/cli Categorizes an issue or PR as relevant to SIG CLI. and removed sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels Oct 4, 2017
@apelisse apelisse mentioned this issue Oct 4, 2017
Merged
k8s-github-robot pushed a commit that referenced this issue Oct 4, 2017
Merge pull request #53232 from apelisse/remove-swagger-validation
4cf9ee3 
Automatic merge from submit-queue (batch tested with PRs 53228, 53232, 53353). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Remove swagger 1.2 validation

**What this PR does / why we need it**:
Removes dependency on swagger 1.2 for validation. Always uses openapi.

cc @mbohlool 

**Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes #44589

**Special notes for your reviewer**:

**Release note**:
```release-note
Kubectl: Remove swagger 1.2 validation. Also removes options `--use-openapi` and `--schema-cache-dir` as these are no longer needed.
```
k8s-github-robot pushed a commit that referenced this issue Oct 4, 2017
Merge pull request #53228 from apelisse/openapi-explain
35ac027 
Automatic merge from submit-queue (batch tested with PRs 53228, 53232, 53353). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Openapi explain

**What this PR does / why we need it**:
This rewrites `kubectl explain` but using openapi rather than swagger 1.2. Also removes the former code.

**Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes #49465, fixes partially #44589, fixes partially #38637

**Special notes for your reviewer**:
FYI @mbohlool 

**Release note**:
```release-note
`kubectl explain` now uses openapi rather than swagger 1.2.
```
@apelisse apelisse mentioned this issue Oct 4, 2017
Merged
k8s-github-robot pushed a commit that referenced this issue Oct 11, 2017
Merge pull request #53441 from apelisse/completely-remove-swagger
24cf4a8 
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

kubectl: Remove swagger 1.2 entirely.

**What this PR does / why we need it**:
Remove dead code since nothing is using swagger 1.2 anymore. This doesn't change any feature, it's just removing unused code.

**Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: Follow up on #44589

**Special notes for your reviewer**:

**Release note**:
```release-note
NONE
```
@clux clux mentioned this issue Nov 3, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@mbohlool mbohlool

@mengqiy mengqiy

Labels
area/kubectl sig/cli Categorizes an issue or PR as relevant to SIG CLI.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Remove swagger 1.2 validation apelisse/kubernetes
7 participants
@earth2marsh @mbohlool @apelisse @mengqiy @0xmichalis @bgrant0607 @pwittrock