Refactor config to be aware of the backend in use #222
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- No real reason for this to happen, once the JSON is loaded, the values aren't changed until API restart and not modified elsewhere currently - It's probably a good thing the request behind ICAT_PROPERTIES actually gets called on demand in case the values get changed before the API is restarted - Ultimately, this is the cause of the issue (requiring DB_URL despite using ICAT backend)
- Following how SciGateway Auth does this, should reduce code in this file
- This commit also removes the old getters, as they're no longer being used
- Similar to what now happens in SciGateway Auth
- These aren't in the current branch, they'll need to be added back when the client handling PR gets merged in
Codecov Report
@@ Coverage Diff @@
## master #222 +/- ##
==========================================
- Coverage 89.72% 89.45% -0.27%
==========================================
Files 31 31
Lines 2346 2296 -50
Branches 188 191 +3
==========================================
- Hits 2105 2054 -51
Misses 211 211
- Partials 30 31 +1
Continue to review full report at Codecov.
|
VKTB
requested changes
Apr 23, 2021
Co-authored-by: Viktor Bozhinov <45173816+VKTB@users.noreply.github.com>
|
@MRichards99 Latest changes look good to me. The conflicts will need to be resolved before I hit the approve button. |
|
The conflicts must've occurred when I merged the other PR into master. I've resolved the conflicts now and got everything working :) |
VKTB
approved these changes
Apr 26, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR will close #210
Description
This is a refactoring for the API's configuration. The root issue described in #210 was caused by calls to config getters in
constants.py- as this file was imported on startup, there was a requirement forDB_URLandICAT_URLto be present. 79d00e8 shows the fix for this root issue, with the commit message giving some further explanation.3c40ccb & 9ad2f57 - I've replaced each of the getter functions with a single, generic getter function, taking inspiration from SciGateway Auth on this. Unlike Auth, the config is not hot reloaded as there's no requirement for the API to have this functionality.
Beforehand, the config options included
ICAT_URLandDB_URL- I've changed these to lowercase snake_case to match the formatting of the others. c1a02e5 for that change.cab681b - a function has been added to check that all relevant config options are present in
config.jsonat startup. This looks at the backend set and determines which of the backend specific config options must be present for all API functionality to work.171dbdf & 76bcb7d - I took some further inspiration from SciGateway Auth and implemented an enum class for the API's config options - as the docstring indicates, this is more for ease of use for development rather than bringing any real functional improvements.
Testing Instructions
I've changed
test_config.pyto reflect these changes, but for manual testing, you should try launching the API with all the config options which startup the API with no issues. Then try removing config option(s) that aren't backend specific, the API should exit when it finds the missing config option, outputting what's missing in the terminal. Do the same for each backend (and their backend specific options), but first set the backend to whichever one you're removing config options from.Agile Board Tracking
Connect to #210