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

Move aai envs from .env to constants file #1262

Merged
merged 3 commits into from
Jul 22, 2024
Merged

Conversation

thePeras
Copy link
Member

Closes #1261

@thePeras thePeras requested a review from a team July 12, 2024 16:55
Copy link

codecov bot commented Jul 12, 2024

Codecov Report

Attention: Patch coverage is 0% with 23 lines in your changes missing coverage. Please review.

Project coverage is 16%. Comparing base (8322384) to head (440302e).

Additional details and impacted files
@@           Coverage Diff           @@
##           develop   #1262   +/-   ##
=======================================
+ Coverage       16%     16%   +1%     
=======================================
  Files          233     233           
  Lines         7219    7214    -5     
=======================================
  Hits          1139    1139           
+ Misses        6080    6075    -5     

@limwa
Copy link
Member

limwa commented Jul 12, 2024

What is the purpose of this PR? Why is this being done?

Personally, I think these values belong in a .env file or something similar.

@limwa
Copy link
Member

limwa commented Jul 12, 2024

Or maybe I should've put it this way: can the notifications worker not access the .env file?

@thePeras
Copy link
Member Author

thePeras commented Jul 15, 2024

@limwa I can explain in the next meeting, but basically in my attempt to load the .env, which was successful, the functions using the environment's variables still doesn't find the DotEnv instantiation. May theory is the package wasn't created for notifications workers. Also, I tried to explore more packages, but no luck. That's why I propose this way, not ideal, but fast, simple and with no security concerns.

Copy link
Contributor

@Process-ing Process-ing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I tested, everything related to the issue seems to be working fine

@thePeras thePeras merged commit 99212cc into develop Jul 22, 2024
6 checks passed
@thePeras thePeras deleted the fix/aai_on_notifications branch July 22, 2024 08:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix notification login with token
4 participants