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

feat(admin_api): support map brackets syntax #13313

Merged
merged 2 commits into from
Jul 10, 2024
Merged

feat(admin_api): support map brackets syntax #13313

merged 2 commits into from
Jul 10, 2024

Conversation

samugi
Copy link
Contributor

@samugi samugi commented Jun 28, 2024 •
edited
Loading

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:

http -f post :8001/plugins/                       \
name=opentelemetry                                \
config.endpoint=http://test.test/                 \
config.resource_attributes[service.name]=kong-dev

Checklist

  • The Pull Request has tests
  • A changelog file has been created under changelog/unreleased/kong or skip-changelog label added on PR if changelog is unnecessary. README.md
  • (no) There is a user-facing docs PR against https://github.com/Kong/docs.konghq.com - PUT DOCS PR HERE

Issue reference

KAG-4827
@samugi samugi marked this pull request as draft June 28, 2024 09:12
@github-actions github-actions bot added core/admin-api cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee labels Jun 28, 2024
@samugi samugi force-pushed the feat/escape-chars-config-keys branch from 579d179 to 86ef821 Compare June 28, 2024 16:03
@samugi samugi changed the title feat(admin_api): support escaped characters in config keys Jun 28, 2024
@samugi samugi force-pushed the feat/escape-chars-config-keys branch from 86ef821 to ab46327 Compare July 1, 2024 10:20
@pull-request-size pull-request-size bot added size/L and removed size/M labels Jul 1, 2024
@samugi samugi requested a review from bungle July 1, 2024 10:21
@samugi samugi force-pushed the feat/escape-chars-config-keys branch 2 times, most recently from 5d3f8ca to 80a992c Compare July 1, 2024 12:20
samugi
samugi commented Jul 1, 2024
View reviewed changes
kong/api/arguments.lua Outdated Show resolved Hide resolved
@samugi samugi marked this pull request as ready for review July 1, 2024 13:33
@samugi samugi requested a review from nowNick July 1, 2024 15:29
@samugi samugi force-pushed the feat/escape-chars-config-keys branch from 4303359 to c3df196 Compare July 2, 2024 15:58
@pull-request-size pull-request-size bot added size/XL and removed size/L labels Jul 2, 2024
nowNick
nowNick reviewed Jul 4, 2024
View reviewed changes
Copy link
Contributor

@nowNick nowNick left a comment •
edited
Loading

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.
kong/api/arguments.lua Outdated Show resolved Hide resolved
kong/api/arguments.lua Outdated Show resolved Hide resolved
kong/api/arguments.lua Outdated Show resolved Hide resolved
kong/api/arguments.lua Outdated Show resolved Hide resolved
@samugi samugi force-pushed the feat/escape-chars-config-keys branch 5 times, most recently from 25351aa to 599f3e1 Compare July 4, 2024 16:58
@samugi
Copy link
Contributor Author

samugi commented Jul 4, 2024 •
edited
Loading

@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.
@samugi samugi requested a review from nowNick July 4, 2024 17:11
kikito
kikito reviewed Jul 5, 2024
View reviewed changes
kong/api/routes/plugins.lua Outdated Show resolved Hide resolved
@samugi
feat(admin_api): support bracket syntax for maps
d69dc69 
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
@samugi samugi force-pushed the feat/escape-chars-config-keys branch from 599f3e1 to f6a13d1 Compare July 5, 2024 12:00
@samugi samugi removed the cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee label Jul 5, 2024
bungle
bungle approved these changes Jul 5, 2024
View reviewed changes
Copy link
Member

@bungle bungle left a 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.
kong/api/arguments.lua Outdated Show resolved Hide resolved
@samugi
refactor(admin_api): arguments
620b016 
* 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
@samugi samugi force-pushed the feat/escape-chars-config-keys branch from f6a13d1 to 620b016 Compare July 5, 2024 15:12
@github-actions github-actions bot added the cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee label Jul 5, 2024
nowNick
nowNick approved these changes Jul 10, 2024
View reviewed changes
Copy link
Contributor

@nowNick nowNick left a 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! 🚀
@samugi samugi removed the cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee label Jul 10, 2024
@samugi samugi merged commit d7cbee6 into master Jul 10, 2024
30 checks passed
@samugi samugi deleted the feat/escape-chars-config-keys branch July 10, 2024 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core/admin-api size/XXL
4 participants
@samugi @kikito @bungle @nowNick