-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Omit gateway tags if empty #4127
Conversation
apidef/oas/server.go
Outdated
@@ -60,6 +60,11 @@ func (s *Server) ExtractTo(api *apidef.APIDefinition) { | |||
} | |||
if s.GatewayTags != nil { | |||
s.GatewayTags.ExtractTo(api) | |||
|
|||
// Omit gateway tags if empty or disabled | |||
if !s.GatewayTags.Enabled || len(s.GatewayTags.Tags) == 0 { |
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.
Please use ShouldOmit function.
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.
Also, nil set should be done after fill function, not extract.
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 just removed a case, because it doesn't work for this structure. I think it doesn't handle the case where the GatewayTags are allocated (Tags field). As we do a nil check, this is type-safe and doesn't use reflection.
Correction: it didn't handle {Enabled: true}...
I added tags validation to require at least 1 array item, verified validation in validation tests. Also removed this from extract and left only in fill as suggested.
API tests result: success ✅ |
API tests result: success ✅ |
API tests result: success ✅ |
API tests result: success ✅ |
API tests result: success ✅ |
API tests result: success ✅ |
💥 CI tests failed 🙈CI test logall ok gofmtall ok goimportsall ok gogenerate:100644 100644 2cf51f5d8c2a45fd6f87883dcf58b4084eb9a19a de2bc36c1eb6a5fc662f8cac1d508b8076b72aae M apidef/oas/schema/schema.gen.go If the above are ok, please look at the run or in the Checks tab. |
API tests result: success ✅ |
API tests result: success ✅ |
API tests result: success ✅ |
💥 CI tests failed 🙈CI test logall ok gofmtall ok goimportsall ok gogenerate:100644 100644 2cf51f5d8c2a45fd6f87883dcf58b4084eb9a19a de2bc36c1eb6a5fc662f8cac1d508b8076b72aae M apidef/oas/schema/schema.gen.go If the above are ok, please look at the run or in the Checks tab. |
bfaed2f
to
58c1aea
Compare
💥 CI tests failed 🙈CI test logall ok gofmtall ok goimportsall ok gogenerate:100644 100644 2cf51f5d8c2a45fd6f87883dcf58b4084eb9a19a de2bc36c1eb6a5fc662f8cac1d508b8076b72aae M apidef/oas/schema/schema.gen.go If the above are ok, please look at the run or in the Checks tab. |
API tests result: success ✅ |
API tests result: success ✅ |
API tests result: success ✅ |
API tests result: success ✅ |
apidef/oas/server.go
Outdated
@@ -42,7 +42,7 @@ func (s *Server) Fill(api apidef.APIDefinition) { | |||
s.GatewayTags = &GatewayTags{} | |||
} | |||
s.GatewayTags.Fill(api) | |||
if ShouldOmit(s.GatewayTags) { | |||
if len(s.GatewayTags.Tags) == 0 { |
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.
Can you please explain why we change 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.
Think that the object will have more fields in future. That’s why we should continue with shouldomit.
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.
shouldOmit
won't omit s.GatewayTags
when s.GatewayTags.Enabled
is true
and len(s.GatewayTags.Tags)
is 0. I think this is required in such cases
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 is a valid case why would you omit if enabled true
. If you don't want 0 length tags with enabled, you would handle it in schema.
API tests result: success ✅ |
apidef/oas/server.go
Outdated
@@ -42,7 +42,7 @@ func (s *Server) Fill(api apidef.APIDefinition) { | |||
s.GatewayTags = &GatewayTags{} | |||
} | |||
s.GatewayTags.Fill(api) | |||
if ShouldOmit(s.GatewayTags) { | |||
if ShouldOmit(s.GatewayTags.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.
it should stay as it is. ShouldOmit(s.GatewayTags)
. I wouldn't push ShouldOmit
if it was just and array length check.
edb9b67
to
9668e64
Compare
API tests result: success ✅ |
API tests result: success ✅ |
e5f8922
to
24218f8
Compare
API tests result: success ✅ |
API tests result: success ✅ |
43dbef2
to
c093096
Compare
Kudos, SonarCloud Quality Gate passed! |
API tests result: success ✅ |
API tests result: success ✅ |
/release to release-4 |
Working on it! Note that it can take a few minutes. |
* Omit gateway tags if empty * Fix existing omit tags case in Fill * Remove some gatewayTags invalid values in tests * Remove second omitEmpty case for gateway tags * Remove allocation of Tags, not required * Disable duplicate test for gatewayTags disabled * Verify gatewayTags array count with schema * Implement furkans suggestions as per PR #4129 * Add test case for Fill/ExtractTo behaviour * Update API schema (go generate) * Add test to validate GatewayTags in Tyk extension * Return enabled:false in oas document, don't omit * Adjust gatewayTags tests to account for ShouldOmit * Remove test, we test for gwtags fill in apidef * Remove duplicate tests for gatewaytags * Remove duplicate tests for gatewaytags * Remove gatewayTags.Tags minItems=1 from validation, pass along as-is, fix tests (cherry picked from commit f86f019)
@titpetric Succesfully merged |
API tests result: success ✅
|
Description
The gateway provided segmentation tags behavior as an always-enabled option. In order to implement a config flag that wouldn't change existing behavior, "tags_disabled" was implemented. When converting between OAS and the Tyk API Definition, this means that the configuration flag for the tags is copied between the two API definitions. This means what is disabled=false for tags in the tyk apidef becomes enabled=true in the OAS document.
This adds a cleanup for the OAS document that removes the gatewayTags entry if either the tags are disabled, or if they are enabled and no tags are defined (length=0).
Related Issue
https://tyktech.atlassian.net/browse/TT-5718
Motivation and Context
How This Has Been Tested
Screenshots (if appropriate)
Types of changes
Checklist
fork, don't request your
master
!master
branch (left side). Also, you should startyour branch off our latest
master
.go mod tidy && go mod vendor
go fmt -s
go vet