Redesign Build Deployment Process (External) #961
Merged
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
added 2 commits
March 21, 2024 11:47
Added a try-catch block similar to how other config files are used in case where actual non-sample file is present else use the sample file. Can remove the file from internal repo since not needed anymore.
Changed level to DEBUG and formatter set to detailed. Can keep both external and internal versions same by making external also have the same logging level as internal. Internal version given priority as detailed logging is better for detecting errors. Also, debug() statements [~1500] are much more than warning() statements [~50]. Will lose out on all these when external repo is used / run if only WARNING level set as default which is higher than DEBUG level.
|
Checking in the first initial set of changes for consolidating the differences in the external and internal versions. Currently included these changes:
|
Push.json.sample just has 4 key-value pairs which can be changed to ENV variables internally. Hence, need to write code to read from Env variables. Changing emission/net/ext_service/push/notify_interface.py to read from env variables. Using helper class config.py, based on existing template. First reading from .json file if exists, else read from env variables. If env variables are all None, then say that values not configured.
Added new environment variable PROD_STAGE. If this is set to TRUE, then the debug.conf.internal.json will be selected as the conf file. Else, the existing try-catch block is executed which either checks for a debug.conf.json or uses the sample file if the former does not exist. The try part is still valid, since some Test files copy over the sample file as debug.conf.json, then reload the config in eac.reload_config() which would need reading from debug.conf.json as well. Hence, now three files exist: - debug.conf.json.sample - debug.conf.json - debug.conf.json.internal
This is the first part of the method to share the server image tag between other repositories. It would be shared as tag.txt, which gets overwritten every time the image build runs.
…jq from docker_start_script.sh Summary: Currently replaced config file values with environment variables. Expecting developers to manually set env vars instead of conf files. So, for instance, running containers using `docker-compose` or `docker run`, need to set these values similarly like push environment variables will need to be set. Key-value pairs in the webserver config file: REMOVED: log_base_dir, python_path, log_file ENV: static_path, 404_redirect, host, port, timeout, auth, aggregate_call_auth ——————————— Changes 1. webserver.conf.sample - Kept file as it is and changed how values being read in cfc_webapp.py, TestWebserver.py and docker_start_script.sh 2. TestWebserver.py - In setup(): Storing current original ENV variables, Replacing them with test values - In teardown(): Restoring original ENV variables 3. cfc_webapp.py - Removed log_base_dir, python_path, log_file as these were only used in the cfc_webapp.py to read in the value from the config file and not used elsewhere. - Added environment variables for the other config key-value pairs to avoid dependence on config file and as they were being used in the cfc_webapp.py file. 4. docker_start_script.sh - Removed sed / jq usage for editing webserver.conf.sample.json file and copying over as webserver.conf.json; simply setting the environment variable for WEB_SERVER_HOST now. ------------------- Special notes: 1. config.json <-> webserver.conf.sample.json - Some files still use config.json and these changes were made 7-8 years ago (even 10 years ago). This config.json file has the same contents as the current webserver.conf.sample. - So, webserver.conf.sample.json was actually config.json at some point! e-mission@a028dec 2. Sed localhost replacement isn’t functionally correct - The default sample value for server.host is "0.0.0.0". - But the sed command replaces “localhost” with the ENV variable value. - Since the localhost keyword itself isn’t there in the sample file, the replacement will not work either. ----------------
Initially, was setting them in the if block if ENV variables already existed. It wasn’t being set as if condition was being evaluated as False. But if condition should be mainly to store existing values. In any case, test values must be set, hence moving it outside if block.
…d / jq use
Details of files changed
1. Start scripts
.docker/docker_start_script.sh
emission/integrationTests/start_integration_tests.sh
setup/tests/start_script.sh
- Changed seq / jq usage to directly set Environment variable to desired value; no need of saving sample file as actual conf json file.
2. Config.py files
emission/core/config.py
emission/net/api/config.py
emission/net/ext_service/push/config.py
- Based this file on emission/analysis/config.py
- Added these to read from conf files if present or environment variables instead of sample files.
- Default values set are taken from sample files.
- check_unset_env_vars() can be used to check whether ALL environment variables are unset.
3. DB, Webapp, Push application usage files
emission/core/get_database.py
emission/net/api/cfc_webapp.py
emission/net/ext_service/push/notify_interface.py
- Changed logic to read using the config.py files that read the non-sample actual config files if present or from the Environment variables instead of sample files.
4. Test Files
emission/integrationTests/storageTests/TestMongodbAuth.py
emission/tests/netTests/TestWebserver.py
emission/tests/netTests/TestPush.py
- Test files that exercise the functionality of the logic in the files in (3).
- Earlier, config files were being replaced with test values and copied over for testing purposes.
- Now, switching to using environment variables - call sent to config files in (2) indirectly via application usage files in (3)
- Following flow is followed in reading from and restoring original environment variables values
setup()
- Sets ENV vars by storing original vars if set, then uses test ENV vars as new values
TestFunc()
- importing modules named in (3) causes values to be read in, which now reads in newer test values since they set the ENV vars in setup()
- only those ENV vars in test values are set; unchanged ones left untouched or default values read using os.getenv(var name, default)
teardown()
- Unset test ENV vars by using del os.environ[var_name]
- Restore original values from original dict
test_run_script_empty () and test_run_script_show() were failing - Was occurring because I had used logging.debug() instead of print() - Hence std output stream was unable to get the print(“storage not configured”) statement Some more fixes: - db.conf incorrectly read instead of webserver.conf in config.py in emisison/net/api - TestPush had an incomplete test, that was used just for testing purposes to invoke calls on importing the pni module. - TestWebserver.py changed print() statements to logging.debug()
Assertion Error this time in another CI GitHub actions Workflow name : test-with-docker Succeeded earlier when the other test (ubuntu-only-test-with-manual-install) failed: https://github.com/e-mission/e-mission-server/actions/runs/8658279606/job/23741854476 - This happened since then “storage not configured” wasn’t being printed, which is what the test wants as docker compose is used to set up mongo db container. - With docker, localhost becomes db as the DB_HOST ENV var or “url” key-value. SOLVED - Added if condition to print “storage not configured” only when url value is till equal to sample value “localhost”
Will first check if debug.conf exists, like other conf files. If it does not, except block then check if PROD_STAGE environment variable is set and whether debug.conf.internal is present. Else, fall back to sample conf.
Try-except block brought to the top
Accidentally added this after running all tests.
Removed extraneous seed_model.json
|
Server testing: First testing the external code as it currently is by building on the current master image. After creating the db:
I built the container as such:
The container spun up and I ran:
Started the e-mission environment:
And ran all the tests:
Every test was passing but one: 
After downloading vim into the container and modifying the file with some prints, I found that the issue was a GET returning a 404. Many trials later, I figured out that the issue was from the docker run command setting STUDY_CONFIG=stage_program, instead of stage-program. After running: All tests passed! 
This is in alignment with the success of test-with-docker and ubuntu-only-test-with-manual-install, which essentially do the same thing. To test the changes that we’ve made, I ran the exact same process as before (without the incorrect environment variable) using the most recently created image from our consolidate-differences image_build_push.yml:
./runAllTests worked for this as well. It is reassuring that the test-with-docker and ubuntu-only-test-with-manual-install runs in our consolidate-differences branch also passed. |
|
@nataliejschultz this only tests previously created images. I don't see you building the images. |
On the image-push-merge branch, we changed the workflows mentioned above to temporarily run on push to image-push-merge branch for testing. Since the actions are running in @MukuFlash03 's forked repository, shouldn't the changes be checked out? This is also how the images with the changes have been pushed; running image_build_push.yml on push to image-push merge on the fork. |
|
Internal repo testing for Analysis: Started mongo container from scratch: Cd into copy of nrelopenpath multi-tier branch and build + run: Checked to make sure that all of the cmd shell scripts made it into the correct directory, based on the new internal configuration: Exec into container, start emission environment, and run ./runAllTests.sh: 
Tests passed. |
added 2 commits
April 25, 2024 02:52
Removed test workflow execution. Added upload artifact.
Commented image build and push for testing purposes. Fixed indentation. Learnt that YAML do not permit using tabs as indentation; spaces allowed.
|
Of note: these are currently the only differences between None of the differences are significant to the functionality of the images. |
|
Recording the deferred cleanup changes from the first round:
|
|
Finished with first round. Going to handle some of the second round of changes because they are annoying me. |
So that we can consolidate all the backwards compat code in one place. if there is a config file and the environment variable is set, we need to decide which one wins. We will pull out the code into a common class to ensure DRY. Once the backwards compat has been removed, this can be merged into the individual config files
|
While working on "there are so many copies of config files; if we do end up with a few, refactor to avoid copy-paste and ensure DRY" I use pandas to retrieve fields from the config file But this then returns the int values wrapped as numpy Making sure to convert to regular int |
|
Finally, I currently pass in both the config file path mappings and the default values And then in the actual usage, we just use the values without defaults That is sub-optimal, since then if the config file is malformed, then we won't have defaults. Since we can retrieve the os environment variables directly, and we are going to retrieve it directly in the future, we should just have the defaults in the main file. That also allows us to avoid passing in two arguments. |
1. This will avoid having multiple, almost identical, copies of the same file 2. It will ensure that we read the code primarily in "the new way" from a dict 3. it should make removing the backwards compat layer easier in the future since we are reading from a dict with a default anyway Testing done: ``` $ ./e-mission-py.bash emission/tests/storageTests/TestTimeSeries.py ---------------------------------------------------------------------- Ran 8 tests in 18.819s OK ```
This allows us to troubleshoot the config files and fix them instead of only falling back to the defaults.
Consistent with e-mission#961 (comment) and 7f1be92 (which was the original example for the database)
Consistent with e-mission#961 (comment) and 7f1be92 (which was the original example for the database) Testing done: 1. Started the webapp with no overridden config ``` Config file not found, returning a copy of the environment variables instead... Finished configuring logging for <RootLogger root (WARNING)> Using auth method skip Replaced json_dumps in plugin with the one from bson Changing bt.json_loads from <function <lambda> at 0x10b2b08b0> to <function <lambda> at 0x10bd6a940> Running with HTTPS turned OFF - use a reverse proxy on production Bottle v0.13-dev server starting up (using CherootServer())... Listening on http://0.0.0.0:8080/ Hit Ctrl-C to quit. ``` 2. Started the webapp with an invalid config override ``` ERROR:root:Expecting ',' delimiter: line 12 column 5 (char 464) Traceback (most recent call last): File "/Users/kshankar/Desktop/data/e-mission/gis_branch_tests/emission/core/backwards_compat_config.py", line 28, in get_config loaded_val = pd.json_normalize(json.load(config_file)).iloc[0] ... json.decoder.JSONDecodeError: Expecting ',' delimiter: line 12 column 5 (char 464) Config file not found, returning a copy of the environment variables instead... ``` 3. Started the webapp with a valid config override ``` Finished configuring logging for <RootLogger root (WARNING)> Using auth method token_list Replaced json_dumps in plugin with the one from bson Changing bt.json_loads from <function <lambda> at 0x10fd2a8b0> to <function <lambda> at 0x1107e5940> Running with HTTPS turned OFF - use a reverse proxy on production Bottle v0.13-dev server starting up (using CherootServer())... Listening on http://0.0.0.0:8080/ Hit Ctrl-C to quit. ```
Consistent with e-mission#961 (comment) and - 3dea305 (example for the api) - 7f1be92 (original example for the database) Testing done: ``` $ ./e-mission-py.bash emission/tests/netTests/TestPush.py ---------------------------------------------------------------------- Ran 5 tests in 0.276s OK ```
if no override is found
…h03/e-mission-server into consolidate-differences
In 7f1be92, we changed the DB configuration to be based on environment variables. This changed the output text when importing the module and launching the script. This change fixes the test token tests that compare the output text with a baseline to reflect the new expected text.
Since we now use the standard backwards compat module (a0f0c6a)
So that we can get a full list of the environment variable by grepping
properly
```
$ grep -r 'config.get("[A-Z]' emission/
emission//net/ext_service/push/notify_interface_impl/firebase.py: self.server_auth_token = push_config.get("PUSH_SERVER_AUTH_TOKEN")
emission//net/ext_service/push/notify_interface_impl/firebase.py: self.app_package_name = push_config.get("PUSH_APP_PACKAGE_NAME")
emission//net/ext_service/push/notify_interface_impl/firebase.py: self.is_fcm_format = push_config.get("PUSH_IOS_TOKEN_FORMAT") == "fcm"
emission//net/ext_service/push/notify_interface.py: return NotifyInterfaceFactory.getNotifyInterface(push_config.get("PUSH_PROVIDER"))
emission//net/api/cfc_webapp.py:server_port = config.get("WEBSERVER_PORT", 8080)
emission//net/api/cfc_webapp.py:socket_timeout = config.get("WEBSERVER_TIMEOUT", 3600)
emission//net/api/cfc_webapp.py:auth_method = config.get("WEBSERVER_AUTH", "skip")
emission//net/api/cfc_webapp.py:aggregate_call_auth = config.get("WEBSERVER_AGGREGATE_CALL_AUTH", "no_auth")
emission//net/api/cfc_webapp.py:not_found_redirect = config.get("WEBSERVER_NOT_FOUND_REDIRECT", "https://nrel.gov/openpath")
emission//core/get_database.py:url = config.get("DB_HOST", "localhost")
emission//core/get_database.py:result_limit = config.get("DB_RESULT_LIMIT", 250000)
```
… runs The tests failed because we now print all environment variables, and the `hostname` changes between runs. So comparing the output with the known good output always fails. We strip out variables that are likely to change over time (hostname, conda version...) to make this test more robust. 1. We technically only need the hostname right now, but will add the conda version as well so that we don't get spurious failures when the versions change 2. this was not an issue earlier because we read the values from the config file. We now read environment variables, but that brings in variables that we did not set. So this is a new issue that we need to resolve by stripping them out for the baseline comparison. ``` $ ./e-mission-py.bash emission/tests/storageTests/TestTokenQueries.py ---------------------------------------------------------------------- Ran 21 tests in 23.591s OK ```
shankari
approved these changes
Aug 12, 2024
emission/net/api/cfc_webapp.py
Outdated
Comment on lines
58
to
64
| static_path = enac.get_config()["static_path"] | ||
| server_host = enac.get_config()["server_host"] | ||
| server_port = enac.get_config()["server_port"] | ||
| socket_timeout = enac.get_config()["socket_timeout"] | ||
| auth_method = enac.get_config()["auth_method"] | ||
| aggregate_call_auth = enac.get_config()["aggregate_call_auth"] | ||
| not_found_redirect = enac.get_config()["not_found_redirect"] |
There was a problem hiding this comment.
Was this ever tested? I don't think that it would have ever worked.
The format of the webserver.conf does not have the static_path at the top level
{
"paths" : {
"static_path" : "webapp/www/",
"python_path" : "main",
"log_base_dir" : ".",
"log_file" : "debug.log"
},
And we didn't modify the path before returning it
try:
config_file = open('conf/net/api/webserver.conf')
ret_val = json.load(config_file)
config_file.close()
except:
|
@MukuFlash03 @nataliejschultz you may want to look through my commits to get a sense of what a clean PR for this change would have looked like. Note that this is not even the fully clean PR but just to the point that was annoying me There are still open issues to be tackled later, but I am out of time to fix this further at this point. |
|
Now there are even more variables. Let's remove them in the test instead of editing the string. Old method of editing the string, for the record |
Consistent with e-mission#961 (comment) Testing done: ``` ---------------------------------------------------------------------- Ran 21 tests in 23.106s OK ```
|
Squash-merging to avoid the multiple back-and-forth changes while polishing this.
|
|
The full set of environment variables supported can be determined by searching for the regular expression I have ensured that this is true now (b6f59b0) and we should make sure to ensure that it is true in the future as well. @MukuFlash03 @nataliejschultz can you please update the docs accordingly? |
14 tasks
|
Cleanup issue e-mission/e-mission-docs#1082 |
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 serves as the implementation for this issue for redesigning our build and deployment processes across e-mission-server, join, admin-dash, public-dash primarily.
Collaborating on this along with @nataliejschultz .
All four redesign PRs for the external repos:
E-mission-server: #961
Join: e-mission/nrel-openpath-join-page#29
Admin-dash: e-mission/op-admin-dashboard#113
Public-dash: e-mission/em-public-dashboard#125
A rough plan can be:
1st phase
2nd phase: