-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
feat(admin_api): support map brackets syntax #13313
Conversation
579d179
to
86ef821
Compare
86ef821
to
ab46327
Compare
5d3f8ca
to
80a992c
Compare
4303359
to
c3df196
Compare
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.
Looks ok and I see a lot of similarities between decode_map_arg
and decode_array_arg
- maybe in future we can use some common logic between those functions. For now I left some refactoring suggestions however I'm not sure if they'll be easy to implement.
25351aa
to
599f3e1
Compare
@nowNick Thanks so much for the great review! My initial objective with this PR was to keep the scope as small as possible, that is why you noticed similarities with existing code, but I have to say that I also agree all your suggestions are improvements. At the same time I don't know how we feel about adding a large refactor on top of this PR, so in the end I did pretty much everything you said (with some additions as I'm about to answer on individual comments) but kept it as a separate commit so we can decide to split it out if needed. I think this is good because the splitting of things resulted in a lot of new unit tests and some more order IMO. |
This commit adds support for configuring maps using brackets syntax to enclose the key: ``` http -f post :8001/plugins/ \ name=opentelemetry \ config.endpoint=http://test.test \ config.resource_attributes[service.name]=kong-dev ``` feat(admin_api): support url encoded brackets
599f3e1
to
f6a13d1
Compare
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.
Looks good. So Approved. I didn't check the refactoring commit though.
* split the arguments module to separate the decoding logic into arguments_decoder.lua * simplified matching logic: extended some regex patterns to cover some of the logic that was done in Lua * split large functions into more smaller ones in arguments_decoder.lua * added unit tests for new functions
f6a13d1
to
620b016
Compare
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.
Looks awesome! Great job! 🚀
Note for reviewers: use hide whitespace if reviewing from gh
Summary
This commit adds support for configuring maps using brackets syntax to
enclose the key:
Checklist
changelog/unreleased/kong
orskip-changelog
label added on PR if changelog is unnecessary. README.mdIssue reference
KAG-4827