-
Notifications
You must be signed in to change notification settings - Fork 17
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
Handle deployment settings in a new collection in the database #716
base: main
Are you sure you want to change the base?
Conversation
…update them in the admin dashboard
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #716 +/- ##
==========================================
- Coverage 66.92% 66.13% -0.80%
==========================================
Files 62 62
Lines 3713 3768 +55
==========================================
+ Hits 2485 2492 +7
- Misses 1228 1276 +48
|
Passing run #1701 ↗︎
Details:
Review all test suite changes for PR #716 ↗︎ |
pre-commit.ci autofix |
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
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.
Looking good, will need to hear more about what @jdbocarsly wants to use this for before deciding whether new settings should be created and whether we need to enumerate any defaults
200, | ||
) | ||
|
||
return (jsonify({"status": "success"}), 200) |
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.
As we discussed, maybe add a route for also creating a new setting that can have a simple string name/value (and maybe description?) plus whether it is public or private as a drop down
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.
Done. For the moment, only the "LOGO_URL" and "HOMEPAGE_URL" settings work. So I also put a dropdown for "name" with only these two entries.
…ing, a 'description' field and model deletion
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
I think much like #604, there's enough to tidy up here (and stuff that could subtly break deployments) that we should hold off until after the 0.4 release to merge this. |
As discussed during the datalab meeting and for future work:
|
Closes #704
Should we have a default value for certain parameters defined in the code? or
Have an empty field in the admin dashboard for parameters not yet defined? or
Just enter them manually in the database the first time?