Skip to content
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

Remove references to environment variables #4633

Merged
merged 24 commits into from
Apr 3, 2023

Conversation

hjoaquim
Copy link
Contributor

@hjoaquim hjoaquim commented Mar 30, 2023

As the title suggest, this PR intends to remove as many references to the environment variables as possible. This allows us:

  1. Greater control over how things change
  2. Centralized approach leveraging SystemModel
  3. Tied with the above: predictability

Functionality check list - from the user standpoint, everything should be the same:

  1. DEBUG_MODE
    a. w/ python terminal.py --debug
    b. openbb
  2. TEST_MODE
    a. Run python terminal.py -t -l (check possible int tests) and then python terminal.py -t 2 --subproc 0 --verbose.
  3. LOG_COLLECTION
    a. if you're a dev and have this flag set to False on the env file, you should see no files on your archives dir on your logs folder.
  4. ENABLE_AUTHENTICATION
    a. if you've it set to Trueon you .env file and use openbb --login you should still be able to use the authenticated functionalities.

All of this can be setup on the .env file and will be initialzed to SystemModel accordingly during the terminal initialization.
DEBUG_MODE and TEST_MODE can obviouly, also be used throught the command line.

Not addressed environment variables that can still be found (use CTRL+shift+F to check it) that we probably want to keep:

  1. PYDEVD_DISABLE_FILE_VALIDATION --> pyinstaller hook
  2. REQUESTS_CA_BUNDLE --> Necessary for installer so that it can locate the correct certificates for API calls and https
  3. SSL_CERT_FILE --> Necessary for installer so that it can locate the correct certificates for API calls and https
  4. SERVER_SOFTWARE --> used for voila dashboards
  5. PYTHONPATH --> used for dashboards

@hjoaquim hjoaquim requested a review from montezdesousa March 30, 2023 10:09
@reviewpad reviewpad bot added the feat S Small T-Shirt size Feature label Mar 30, 2023
@hjoaquim hjoaquim changed the title Remove mentions to environment variables Remove references to environment variables Mar 30, 2023
@montezdesousa
Copy link
Contributor

montezdesousa commented Mar 30, 2023

Number 1: --debug flag seems to disable rich console formatting
Screenshot 2023-03-30 at 16 19 58

Number 2: integration is not launching
Screenshot 2023-03-30 at 16 34 47

Number 4. : openbb --login is also not launching the terminal with OPENBB_ENABLE_AUTHENTICATION='True' in the .env

@montezdesousa
Copy link
Contributor

There are a few load_env_vars references which are loading environment variables as well. Should this go into the SystemModel as well?

@hjoaquim
Copy link
Contributor Author

@montezdesousa would you give it another try (maybe on a fresh env)?

Everything seems alright on my end:
1.
image
image
2.
image
image
4.
image

@montezdesousa
Copy link
Contributor

@montezdesousa would you give it another try (maybe on a fresh env)?

Everything seems alright on my end: 1. image image 2. image image 4. image

You are right, don't know what was happening there!

@hjoaquim
Copy link
Contributor Author

load_env_vars

Good one. Was able to remove the function completely!

@hjoaquim hjoaquim requested a review from montezdesousa April 1, 2023 17:24
@codecov
Copy link

codecov bot commented Apr 3, 2023

Codecov Report

❗ No coverage uploaded for pull request base (develop@405c7d6). Click here to learn what that means.
Patch has no changes to coverable lines.

❗ Current head ff9275d differs from pull request most recent head 0f007f6. Consider uploading reports for the commit 0f007f6 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             develop    #4633   +/-   ##
==========================================
  Coverage           ?   55.99%           
==========================================
  Files              ?      591           
  Lines              ?    53957           
  Branches           ?        0           
==========================================
  Hits               ?    30212           
  Misses             ?    23745           
  Partials           ?        0           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@hjoaquim
Copy link
Contributor Author

hjoaquim commented Apr 3, 2023

Related with: #4665
@montezdesousa after this PR gets merged, we should use the auth flag on the SystemModel to control the is_auth_enabled function

@montezdesousa
Copy link
Contributor

Related with: #4665 @montezdesousa after this PR gets merged, we should use the auth flag on the SystemModel to control the is_auth_enabled function

What's left to do before merging?

@hjoaquim
Copy link
Contributor Author

hjoaquim commented Apr 3, 2023

Related with: #4665 @montezdesousa after this PR gets merged, we should use the auth flag on the SystemModel to control the is_auth_enabled function

@montezdesousa did it here: 0f007f6

@hjoaquim
Copy link
Contributor Author

hjoaquim commented Apr 3, 2023

Related with: #4665 @montezdesousa after this PR gets merged, we should use the auth flag on the SystemModel to control the is_auth_enabled function

What's left to do before merging?

everything's finished now, I think!

@montezdesousa montezdesousa enabled auto-merge April 3, 2023 16:10
@montezdesousa montezdesousa added this pull request to the merge queue Apr 3, 2023
Merged via the queue into OpenBB-finance:develop with commit 13a91af Apr 3, 2023
@hjoaquim hjoaquim deleted the hotfix/remove-envs branch April 3, 2023 16:38
jmaslek pushed a commit that referenced this pull request Apr 13, 2023
* removing references to test_mode as env variable

* removing references to os.getenv

* adding functions to change system properties

* removing references to env

* introducing set_system_variable and removing change_logging_suppress function

* removing change_test_mode and change_debug_mode

* removing change_log_collect

* adjusting config_terminal and generate_sdk

* generating the sdk

* removing unused imoports

* introducing DISABLE_STREAMLIT_WARNING

* removing load_env_vars

* test_print_help test

* sync tests

* rewrite tests

* setting the auth to True as default

---------

Co-authored-by: montezdesousa <79287829+montezdesousa@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat S Small T-Shirt size Feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants