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

Minor consolidation of Environment Variables #614

Merged
merged 16 commits into from
Jan 17, 2025

Conversation

njbrake
Copy link
Contributor

@njbrake njbrake commented Jan 13, 2025

What's changing

This pull request involves several changes to update environment configuration files and improve the setup for local development and testing. The key changes include renaming the environment template file, updating references across the codebase, and modifying environment variable defaults.

Environment Configuration Updates:

  • .env.example renamed to .env.template and updated with additional comments and placeholders for various configuration sections. [1] [2]

Makefile Adjustments:

  • Updated Makefile to reference the new .env.template file instead of the old .env.example file.
  • Added necessary environment variables for backend unit and integration tests in the Makefile.

Documentation Update:

  • Updated the reference to the environment file in docs/source/user-guides/inference.md from .env.example to .env.template.

Backend Settings Update:

  • Modified lumigator/python/mzai/backend/backend/settings.py to remove default values for S3_BUCKET, RAY_HEAD_NODE_HOST, and RAY_DASHBOARD_PORT, indicating that these should be specified in the .env file.

How to test it

Steps to test the changes:

  1. Run make local-up, check to see if .env file variables are correctly loaded.

Additional notes for reviewers

The idea of this PR is that now the environment variable file is only used for overrides, not for regular settings.

I already...

  • Tested the changes in a working environment to ensure they work as expected
  • Added some tests for any new functionality
  • Updated the documentation (both comments in code and product documentation under /docs)
  • Checked if a (backend) DB migration step was required and included it if required

@njbrake njbrake linked an issue Jan 13, 2025 that may be closed by this pull request
1 task
@github-actions github-actions bot added documentation Improvements or additions to documentation backend labels Jan 13, 2025
@njbrake njbrake changed the title .env is for overrides only Consolidation of Environment Variables Jan 13, 2025
@njbrake njbrake marked this pull request as draft January 13, 2025 18:46
@njbrake njbrake changed the title Consolidation of Environment Variables Minor consolidation of Environment Variables Jan 14, 2025
@njbrake njbrake marked this pull request as ready for review January 14, 2025 18:31
Copy link
Contributor

@javiermtorres javiermtorres left a comment

Choose a reason for hiding this comment

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

LGTM. I'll wait for other opinions before approving, though.
I agree with keeping defaults in a single point, and adding configuration if needed.

@njbrake njbrake requested a review from javiermtorres January 15, 2025 15:03
@agpituk
Copy link
Contributor

agpituk commented Jan 16, 2025

I think this is a great consolidation PR @njbrake ! Just added a minor comment on the Makefile. Also, what do you think about also having the SQLALCHEMY_DATABASE_URL in the .env file instead of the docker-compose.yaml? in order to consolidate things in one place.

@njbrake
Copy link
Contributor Author

njbrake commented Jan 16, 2025

I think this is a great consolidation PR @njbrake ! Just added a minor comment on the Makefile. Also, what do you think about also having the SQLALCHEMY_DATABASE_URL in the .env file instead of the docker-compose.yaml? in order to consolidate things in one place.

@agpituk would it be ok if we push that change off into a future change? Tbh I'm not totally sure how the database setup works yet, and that env var shows up in enough places that I'm not yet confident that I won't accidentally break something when trying to change it 😆 .

@agpituk
Copy link
Contributor

agpituk commented Jan 17, 2025

I think this is a great consolidation PR @njbrake ! Just added a minor comment on the Makefile. Also, what do you think about also having the SQLALCHEMY_DATABASE_URL in the .env file instead of the docker-compose.yaml? in order to consolidate things in one place.

@agpituk would it be ok if we push that change off into a future change? Tbh I'm not totally sure how the database setup works yet, and that env var shows up in enough places that I'm not yet confident that I won't accidentally break something when trying to change it 😆 .

definitely! :) I'll open an issue so we don't forget. Thanks @njbrake

Makefile Show resolved Hide resolved
@njbrake njbrake requested a review from agpituk January 17, 2025 13:22
@njbrake njbrake merged commit 710571a into main Jan 17, 2025
13 checks passed
@njbrake njbrake deleted the 609-bug-env-file-is-being-ignored branch January 17, 2025 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend documentation Improvements or additions to documentation frontend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG]: .env file is being ignored
3 participants