-
Notifications
You must be signed in to change notification settings - Fork 40
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
fix: fill defaults for non-required field of type "record" #336
fix: fill defaults for non-required field of type "record" #336
Conversation
As acme plugin's schema has varied a lot over different versions of kong, it is easier to handle if this is added to the test itself
thanks @nishant95! I just tested your branch and I still have the same issue, my field is set to |
@aboudreault Yes, I raised this PR for issue #334 , this deals with record type only. But shouldn't there be a similar handling for type "map" as well? Similar to this in Lua. |
@nishant95 this is possible yeah. Let's try to release this fix and #333, I will create a ticket for the map type. |
Overriding linter failure and merging cause it ain't worth waiting to round trip to the upstream branch and the error is inscrutable:
golangci, please just print the actual command you need to run. Furthermore, the import block didn't change between those commits; it shouldn't start failing. This is not an import >:(
Going to fix in the release changelog commit. |
@@ -1671,3 +2079,184 @@ func Test_FillPluginsDefaults_SetType(t *testing.T) { | |||
}) | |||
} | |||
} | |||
|
|||
func Test_FillPluginsDefaults_Acme(t *testing.T) { | |||
RunWhenKong(t, "==3.3.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.
Is there a reason we want to run this against 3.3 only? If so then I believe this line deserves a comment why that's the case. It's not obvious to the reader why this particular version was chosen here and deserves a special treatment.
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.
@pmalek You are right, this check is not needed. This is a residual check which I added initially as the schema for this plugin changed a lot over different kong versions, this is of no use now as the schema has been added to the test itself.
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! This should fix it then right? #342
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.
No, this should fix it: #343 Else the test would fail for many of the kong versions as the schema for Acme plugin would be different.
Fixes #334
Adds a check for required while recusrively filling defaults for subconfig of type record.
Current behaviour of KIC is out of sync from Kong Lua implementation.
There are some field "required" checks in Lua code: here and here
These checks are missing form go code