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

fix: always use API_V3_URL environment variable #2178

Merged
merged 1 commit into from
Aug 30, 2024

Conversation

sloanelybutsurely
Copy link
Contributor

@sloanelybutsurely sloanelybutsurely commented Aug 30, 2024

Our ECS tasks all set a API_V3_URL pointing to the desired V3 API environment but we were ignoring that value and instead deriving the V3 API URL based on ENVIRONMENT_NAME. This is confusing.

With this change the URL used for the V3 API is always fetched from the environment as API_V3_URL (unless MIX_ENV=test).

This has the added benefit of picking up the changes in https://github.com/mbta/devops/pull/2122

Asana task: Point screens-dev-green at api-dev-blue

@sloanelybutsurely sloanelybutsurely requested a review from a team as a code owner August 30, 2024 20:23
@digitalcora
Copy link
Contributor

Seems odd that we have two ways of doing this. If we have API_V3_URL in deployed environments, any reason we shouldn't just always look at that, and delete this extra condition based on ENVIRONMENT_NAME?

@sloanelybutsurely
Copy link
Contributor Author

@digitalcora that's a great point! we do have this as an environment variable in prod, dev, and dev-green. i'll go ahead and refactor this code to always use API_V3_URL.


the signs_ui s3 bucket is also configured based on ENVIRONMENT_NAME but those values are available as a terraform variable (would require exposing them via environment variable in the ECS task definition). i'll open separate PRs for those changes.

signs_ui_s3_bucket =
case eb_env_name do
"screens-prod" -> "mbta-signs"
"screens-dev" -> "mbta-signs-dev"
"screens-dev-green" -> "mbta-signs-dev"
_ -> nil
end

@digitalcora
Copy link
Contributor

I suppose we could also say that ENVIRONMENT_NAME is the one true way, and derive everything from that. Advantage would be that it doesn't require Terraform changes to change how our environments are connected. But then that's kind of putting "infrastructural" data in the app code.

@sloanelybutsurely sloanelybutsurely changed the title chore: point screens-dev-green at api-dev-blue fix: always use API_V3_URL environment variable Aug 30, 2024
@sloanelybutsurely
Copy link
Contributor Author

I suppose we could also say that ENVIRONMENT_NAME is the one true way, and derive everything from that. Advantage would be that it doesn't require Terraform changes to change how our environments are connected. But then that's kind of putting "infrastructural" data in the app code.

@digitalcora my preference is to always use the environment variables for each individual parameter. this keeps that kind of configuration in terraform as a single source of truth for connections between applications

config/runtime.exs Outdated Show resolved Hide resolved
Our ECS tasks all set a `API_V3_URL` pointing to the desired V3 API
environment but we were ignoring that value and instead deriving the V3
API URL based on `ENVIRONMENT_NAME`. This is confusing.

With this change the URL used for the V3 API is always fetched from the
environment as `API_V3_URL` (unless `MIX_ENV=test`).

This has the added benefit of picking up the changes in mbta/devops#2122
Copy link
Contributor

@cmaddox5 cmaddox5 left a comment

Choose a reason for hiding this comment

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

Sorry for missing what Cora caught. And thanks for fixing it.

@sloanelybutsurely sloanelybutsurely merged commit 68c98b8 into main Aug 30, 2024
11 checks passed
@sloanelybutsurely sloanelybutsurely deleted the sloane/push-xuknkrvpkqnz branch August 30, 2024 21:18
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.

3 participants