-
Notifications
You must be signed in to change notification settings - Fork 2.7k
fix(ai-proxy): correct logging schema key in ai-proxy-multi #12795
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
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Orician <2018783812@qq.com>
Baoyuantop
left a comment
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.
We need to add additional tests to check the schema configuration in order to avoid encountering similar issues again.
Test Summary Report
-------------------
t/plugin/mcp-bridge.t (Wstat: 0 Tests: 10 Failed: 3)
Failed tests: 7-9
Parse errors: No plan found in TAP output
Files=5, Tests=182, 56 wallclock secs ( 0.04 usr 0.01 sys + 4.80 cusr 2.30 csys = 7.15 CPU)
Result: FAILI don't know why the mcp-bridge test failed, it should have nothing to do with my modification. |
Ignoring this test for now, the same issue exists in other PRs. |
Hi @yixianOu, could you add a simple test case? |
ok |
…expect unknown logging_schema to be ignored (ok:ok). Also remove error.log requirement in schema test to match validator behavior.
|
add additional tests to check the schema configuration |
t/plugin/ai-proxy.t
Outdated
| -- APISIX schema allows additional properties unless explicitly disallowed. | ||
| -- So unknown field 'logging_schema' should be ignored and still pass. | ||
| ngx.say((ok1 and "ok" or ("bad:" .. (err1 or ""))), ":", (ok2 and "ok" or "invalid")) |
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 too complicated. Please use if else statements for easier code understanding.
t/plugin/ai-proxy-multi.t
Outdated
|
|
||
| __DATA__ | ||
| === TEST 0: schema accepts 'logging' and ignores unknown 'logging_schema' |
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.
We should add it to the end of the existing test.
|
https://github.com/apache/apisix/actions/runs/20191178102/job/58038560408?pr=12795
|
operators with if-else statements - Replace complicated nested ternary operators in TEST 17 with clear if-else blocks - Improve code readability and maintainability in t/plugin/ai-proxy.t - Reindex test file for proper formatting - All linting checks pass Signed-off-by: Orician <2018783812@qq.com>
Description
This PR unifies the schema definition in the
ai-proxy-multiplugin. Previously, theai_proxy_multi_schemausedlogging_schemaas the key name, which was inconsistent with theai-proxyplugin (which useslogging). This change ensures the logging configuration key is consistent across AI proxy plugins.Checklist
Related Issue
Fixes #12671
Signed-off-by: Orician 2018783812@qq.com