-
Notifications
You must be signed in to change notification settings - Fork 542
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: support return the manager api's git hash and version #1408
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1408 +/- ##
==========================================
+ Coverage 45.15% 45.20% +0.05%
==========================================
Files 35 37 +2
Lines 2516 2524 +8
==========================================
+ Hits 1136 1141 +5
- Misses 1215 1218 +3
Partials 165 165
Continue to review full report at Codecov.
|
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 a better title, it confused me
@starsz try to sync with the latest codes from the master branch? |
It's already the latest. Please see the CI on the master branch. |
fine, then please request changes from other members, once the FE test is fixed, I will let you know. |
FE is working on why test failed, once test fixed, we will merge. |
} | ||
|
||
func (h *Handler) ApplyRoute(r *gin.Engine) { | ||
r.GET("/version", wgin.Wraps(h.Version)) |
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 will lead to 2 kinds of APIs:
/apisix/admin/xxx
/version
When we deprecate admin-api
in V3, we will have to omit that prefix apisix/admin
either, to have ManagerAPIs.
Now FE meets one issue: because HTTP call is handled by UmiJS, which only supports one prefix, /apisix/admin
or /
, then FE has to make hack codes to be compatible with those 2 kinds of APIs.
I know it's wired to have something like /apisix/admin/version
, but could we add a prefix for this API first? with some description about this hacking case. In V3, we may omit that prefix from all APIs.
cc @LiteSun
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.
OK.Got it.
I will add the prefix and ignore this API's auth checking in the next PR.
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'm not sure if we need to omit the prefix, but unification is appropriate.
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.
Thanks!!!
Please answer these questions before submitting a pull request
Why submit this pull request?
[] Bugfix
New feature provided
Improve performance
Backport patches
Related issues
fix #1404
New feature or improvement
Add
info
API to support get the backend info.