-
Notifications
You must be signed in to change notification settings - Fork 10
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
chore: replaced kin-openapi library with libopenapi #194
Conversation
e6359e0
to
fcd8764
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #194 +/- ##
==========================================
+ Coverage 67.86% 68.19% +0.33%
==========================================
Files 23 24 +1
Lines 2505 2550 +45
==========================================
+ Hits 1700 1739 +39
+ Misses 644 643 -1
- Partials 161 168 +7 ☔ View full report in Codecov by Sentry. |
deck file openapi2kong command uses kin-openapi for all its OpenAPI requirements. This library does not support OpenAPI 3.1, which is a requirement from the users. Thus, this change updates the library to libopenapi which can help us to adopt OpenAPI 3.1 For APIOps #1324 Kong/deck#1324
b8cf0bf
to
3c66c66
Compare
@@ -157,6 +157,7 @@ func (patch *DeckPatch) ApplyToNodes(yamlData *yaml.Node) (err error) { | |||
logbasics.Info("Patch has no selectors specified") | |||
} | |||
|
|||
//nolint:gosimple |
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.
Unrelated to my diff.
Thus, I have added a nolint rule here.
@@ -46,6 +46,7 @@ func NewSelectorSet(selectors []string) (SelectorSet, error) { | |||
|
|||
// IsEmpty returns true if the selector set is empty. | |||
func (set *SelectorSet) IsEmpty() bool { | |||
//nolint:gosimple |
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.
Unrelated to my diff.
Thus, I have added a nolint rule here.
Same for L71
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 work @Prashansa-K !! Added some minor comments. Note that I don't know this go-apiops library so I think this work seems safe if the test suite and coverage are good.
@@ -15,6 +15,7 @@ func ApplyNamespaceHost( | |||
deckfile *yaml.Node, // the deckFile to operate on | |||
selectors yamlbasics.SelectorSet, // the selectors to use to select the routes | |||
hosts []string, // the hosts to add to the routes | |||
//nolint:predeclared |
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.
CI doesn't seem to complain, but locally I am getting this error:
namespace/namespace_host.go:18:2: directive `//nolint:predeclared` is unused for linter "predeclared" (nolintlint)
//nolint:predeclared
``
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 could be because of the differences in the golangci-lint version we are using in the CI and the one used locally.
Let me check with same versions, because I added all the //nolint comments after checking the CI. I wasn't getting any warnings in my local setup.
openapi2kong/jsonschema.go
Outdated
for _, schema := range s.AllOf { | ||
dereferenceSchema(schema, seenBefore) | ||
} | ||
|
||
for _, schema := range s.AnyOf { | ||
dereferenceSchema(schema, seenBefore) | ||
} | ||
|
||
for _, schema := range s.OneOf { | ||
dereferenceSchema(schema, seenBefore) | ||
} |
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: could probably also be a single for as before
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.
The types had changed in the two underlying libraries.
In kin-openapi (older lib), s.AllOf
and others were of type SchemaProxy
In the new one, all these are of type []*SchemaProxy
So, I had to use different for loops. Though, I have changed them to a nested one now. Makes things more readable.
openapi2kong/jsonschema.go
Outdated
finalSchema := make(map[string]interface{}) | ||
|
||
if s.IsReference() { | ||
// Add ref key and string |
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: I would avoid comments that don't add more useful details than what we can read from the code itself.
openapi2kong/openapi2kong.go
Outdated
default: | ||
return nil, fmt.Errorf("expected 'x-kong-tags' to be an array of strings") | ||
kongTags, ok := doc.Extensions.Get("x-kong-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.
openapi2kong/openapi2kong.go
Outdated
for i := 0; i < len(tagsArray); i++ { | ||
switch tag := tagsArray[i].(type) { | ||
resultArray := make([]string, len(kongTags.Content)) | ||
|
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.
} | ||
|
||
var jsonBlob interface{} | ||
_ = yaml.Unmarshal(xKongObjectBytes, &jsonBlob) |
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.
any particular reason to not handle the error here?
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 because we error out in the upcoming line where we check it the jsonBlob can be safely converted to an object of type map[string]interface{}. If not, we are returning a user-specific error.
default: | ||
return nil, fmt.Errorf("expected '/components/x-kong' to be a JSON object") | ||
} | ||
if !ok || xKongComponents == nil { |
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.
same question about the !ok
handling
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 similar to the above !ok handling.
A component may not have any extensions added via x-kong
Here's an example from this repo itself: https://github.com/Kong/go-apiops/blob/main/docs/mock-a-rena-oas.yml
This spec doesn't use any extensions (top level or component level), but it is still a valid spec and we can generate its equivalent kong config.
} | ||
|
||
var xKong interface{} | ||
_ = yaml.Unmarshal(xKongComponentsBytes, &xKong) |
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.
same question about the error handling
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.
Same reason as above error handling comment. I have changed this one's handling to how we handle things above as well.
openapi2kong/openapi2kong.go
Outdated
var pluginConfig map[string]interface{} | ||
err = json.Unmarshal(jsonstr, &pluginConfig) | ||
if err != nil { | ||
return nil, fmt.Errorf(fmt.Sprintf("failed to parse JSON object for '%s': %%w", extensionName), err) |
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.
return nil, fmt.Errorf(fmt.Sprintf("failed to parse JSON object for '%s': %%w", extensionName), err) | |
return nil, fmt.Errorf("failed to parse JSON object for '%s': %w", extensionName, err) |
openapi2kong/openapi2kong.go
Outdated
kongComponents, kongTags, opts.SkipID) | ||
if err != nil { | ||
return nil, fmt.Errorf("failed to create plugins list from document root: %w", err) | ||
} | ||
|
||
// get the OIDC stuff from top level, bail out if the requirements are unsupported | ||
// // get the OIDC stuff from top level, bail out if the requirements are unsupported |
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.
// // get the OIDC stuff from top level, bail out if the requirements are unsupported | |
// get the OIDC stuff from top level, bail out if the requirements are unsupported |
Thank you! I will also test this out with some more examples to be thorough. |
This version of go-apiops has replaced kin-openapi with libopenapi to add further support of OpenAPI 3.1 Refer: Kong/go-apiops#194 This change would not affect users using OpenAPI 3.0 spec with deck command. Fix for #1324: #1324
This version of go-apiops has replaced kin-openapi with libopenapi to add further support of OpenAPI 3.1 Refer: Kong/go-apiops#194 This change would not affect users using OpenAPI 3.0 spec with deck command. Fix for #1324: #1324
This version of go-apiops has replaced kin-openapi with libopenapi to add further support of OpenAPI 3.1 Refer: Kong/go-apiops#194 This change would not affect users using OpenAPI 3.0 spec with deck command. Fix for #1324: #1324
deck file openapi2kong command uses kin-openapi
for all its OpenAPI requirements. This library
does not support OpenAPI 3.1, which is a
requirement from the users.
Thus, this change updates the library to libopenapi which can help us to adopt OpenAPI 3.1
For APIOps #1324
Kong/deck#1324
I didn't add much code refactoring in this PR for the aid of the review. I will take that up in a different PR.