-
Notifications
You must be signed in to change notification settings - Fork 222
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
refactor(ui): stop using /meta/config endpoint #3684
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3684 +/- ##
==========================================
+ Coverage 64.41% 64.56% +0.15%
==========================================
Files 170 170
Lines 17098 17132 +34
==========================================
+ Hits 11013 11061 +48
+ Misses 5400 5382 -18
- Partials 685 689 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
ec271f2
to
48b800c
Compare
@markphelps I have to add few tests but I looking for feedback on this. |
48b800c
to
0bfccce
Compare
@@ -3,19 +3,27 @@ package info | |||
import ( | |||
"encoding/json" | |||
"net/http" | |||
|
|||
"go.flipt.io/flipt/internal/config" | |||
) | |||
|
|||
type Flipt struct { |
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 have an opportunity to refactor this info API endpoint now I think?
maybe we should make the response it a bit more structured, something like:
{
version: {
current:
latest: {
version:
URL:
},
build: {
date: ,
commit:
goVersion:
release:
},
authentication: {
required:
},
analytics: {
enabled:
},
storage: {
...
},
}
this would be an API response breaking change, but since the API is not documented for this endpoint maybe its ok?
wdyt?
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.
@markphelps I’ve updated the new fields to match the desired format. However, I'm not ready to update old fields in this PR, as it would introduce too many changes at once.
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.
That sounds good to me
0bfccce
to
9cf73cd
Compare
Signed-off-by: Roman Dmytrenko <[email protected]>
9cf73cd
to
c224ce5
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.
this looks great! thank you @erka
c224ce5
to
929704f
Compare
Signed-off-by: Roman Dmytrenko <[email protected]>
/meta/config
exposes too much configuration and UI doesn't need that much. There were few leakage of the secrets as people provide configuration options in their way and that fire seem never to stop.The global idea is to drop
/meta/config
endpoint and this is a first step to it.