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

Feat: customize the login form using an environment variable #2361

Merged
merged 1 commit into from
Sep 12, 2024

Conversation

lalver1
Copy link
Member

@lalver1 lalver1 commented Sep 11, 2024

This PR sets the login form configuration using an environment variable.
Before merging this PR, ensure that:

  • dev has a new secret in the key vault and a new environment variable that refers to this secret in the app settings
  • test has a new secret in the key vault and a new environment variable that refers to this secret in the app settings
  • prod has a new secret in the key vault and a new environment variable that refers to this secret in the app settings

@lalver1 lalver1 self-assigned this Sep 11, 2024
@github-actions github-actions bot added deployment-dev [auto] Changes that will trigger a deploy if merged to dev back-end Django views, sessions, middleware, models, migrations etc. infrastructure Terraform, Azure, etc. labels Sep 11, 2024
Copy link

github-actions bot commented Sep 11, 2024

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  benefits
  settings.py
Project Total  

This report was generated by python-coverage-comment-action

@lalver1 lalver1 marked this pull request as ready for review September 11, 2024 20:46
@lalver1 lalver1 requested a review from a team as a code owner September 11, 2024 20:46
@@ -72,7 +72,7 @@ def RUNTIME_ENVIRONMENT():
"https://www.googleapis.com/auth/userinfo.email",
"https://www.googleapis.com/auth/userinfo.profile",
]
SSO_SHOW_FORM_ON_ADMIN_PAGE = False
SSO_SHOW_FORM_ON_ADMIN_PAGE = os.environ.get("SSO_SHOW_FORM_ON_ADMIN_PAGE", "False").lower() == "true"
Copy link
Member

Choose a reason for hiding this comment

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

machikoyasuda
machikoyasuda previously approved these changes Sep 11, 2024
Copy link
Member

@machikoyasuda machikoyasuda left a comment

Choose a reason for hiding this comment

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

Tested:
image

Copy link
Member

@angela-tran angela-tran left a comment

Choose a reason for hiding this comment

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

This LGTM!

Copy link
Member

@angela-tran angela-tran left a comment

Choose a reason for hiding this comment

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

Luis noticed that there is a missing comma in the line above the one he added 😅 so I'm dismissing my review

@lalver1 lalver1 force-pushed the feat/sso-form-setting branch from f87965b to 27a98fb Compare September 12, 2024 16:19
@lalver1
Copy link
Member Author

lalver1 commented Sep 12, 2024

It's interesting @angela-tran , looks like commas are not required according to the terraform docs since app_settings are a key-value pair, but it's prob best to include them (it also makes the VS Code syntax highlighting look uniform).

@lalver1 lalver1 merged commit e29074e into main Sep 12, 2024
15 checks passed
@lalver1 lalver1 deleted the feat/sso-form-setting branch September 12, 2024 16:33
@angela-tran angela-tran linked an issue Sep 12, 2024 that may be closed by this pull request
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
back-end Django views, sessions, middleware, models, migrations etc. deployment-dev [auto] Changes that will trigger a deploy if merged to dev infrastructure Terraform, Azure, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow native login if needed
3 participants