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

chore: use SIGNS_UI_S3_BUCKET environment variable #2180

Merged
merged 1 commit into from
Sep 5, 2024

Conversation

sloanelybutsurely
Copy link
Contributor

@sloanelybutsurely sloanelybutsurely commented Aug 30, 2024

References the new environment variable from https://github.com/mbta/devops/pull/2129 instead of deriving this value based on ENVIRONMENT_NAME

Warning

https://github.com/mbta/devops/pull/2129 must be deployed first! Update: This is done ✅

@cmaddox5
Copy link
Contributor

cmaddox5 commented Sep 3, 2024

A small thing I noticed. SignsUI dev and dev-green share the same bucket but use different config files (which we store in config.exs under :signs_ui_s3_path), with the only difference being dev -> config.json and dev-green -> config-dev-green.json. Can we also make the change to reflect that here?

@digitalcora
Copy link
Contributor

Given the existing logic was to always use the config for dev and not dev-green when pointed at the "dev bucket", I'd be fine with leaving that as it is for this change — no need to introduce more environment variables / configuration unless we have a known use case for them.

References the new environment variable from
mbta/devops#2129 instead of deriving this value
based on `ENVIRONMENT_NAME`
@sloanelybutsurely
Copy link
Contributor Author

SignsUI dev and dev-green share the same bucket but use different config files (which we store in config.exs under :signs_ui_s3_path), with the only difference being dev -> config.json and dev-green -> config-dev-green.json.

@cmaddox5 Where do you see this? I only see one place where signs_ui_s3_path is set.

signs_ui_s3_path: "config.json",

🔍 Code search for signs_ui_s3_path

@cmaddox5
Copy link
Contributor

cmaddox5 commented Sep 4, 2024

@sloanelybutsurely I was referencing the actual config name for each environment in SignsUI. So technically, we aren't matching up our dev-green environment to the correct config. But I agree with Cora, this is a different issue so it can be deferred to a later change if needed.

@sloanelybutsurely
Copy link
Contributor Author

Just waiting on the infra changes to be applied to prod before merging

@sloanelybutsurely sloanelybutsurely merged commit 81d474a into main Sep 5, 2024
14 checks passed
@sloanelybutsurely sloanelybutsurely deleted the sloane/push-tszzpmrqyqmp branch September 5, 2024 20:50
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