-
Notifications
You must be signed in to change notification settings - Fork 2
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
Prepare for integration with infra template #40
Conversation
This breaks the infra template integration.
This reverts commit c2cc7e5.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work!
Left a few comments, the bucket one needs to be addressed before merging.
AWS_COGNITO_USER_POOL_ID=<FILL ME IN> | ||
AWS_COGNITO_CLIENT_ID=<FILL ME IN> | ||
AWS_COGNITO_CLIENT_SECRET=<FILL ME IN> | ||
COGNITO_USER_POOL_ID=<FILL ME IN> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you also mean to remove AWS
from line 23, in AWS_BUCKET_NAME=<FILL ME IN>
because you refer to `ENV["BUCKET_NAME"] elsewhere?
Also, maybe take it out of the AWS Services section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Thank you!
@@ -14,7 +14,7 @@ def initialize(client: Aws::CognitoIdentityProvider::Client.new) | |||
def create_account(email, password) | |||
begin | |||
response = @client.sign_up( | |||
client_id: ENV["AWS_COGNITO_CLIENT_ID"], | |||
client_id: ENV["COGNITO_CLIENT_ID"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if this is the right rails way, but could we set this client_id, along with the user_pool_id, from the ENV at the top of the class and reuse the variable, instead of resetting it in each method to be more DRY?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a good change, but I'd like to break it out into a separate issue.
@@ -61,7 +60,7 @@ To preview email views in the browser, visit: `/rails/mailers` | |||
|
|||
To test AWS SES email sending locally: | |||
|
|||
1. Set the `AWS_*` env var in your `.env` file | |||
1. Set the "AWS services" env var in your `.env` file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
env vars?
env variables?
Since it is multiple vars, it's something someone might get tripped up on if they see it singular here.
If aws bucket (now just bucket) is moved out of AWS services section, is it needed here for SES to work properly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe so.
## Requirements | ||
|
||
- External service access: The application must be able to make network calls to the public internet. | ||
- AWS Cognito: The application comes with AWS Cognito enabled by default, so AWS Cognito must be set up before deploying the application. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AWS Cognito has a lot of settings/defaults, and it's not clear from this how to set up a cognito user pool to satisfy the application. A terrafrom module would go a long way to describing what an example cognito/user pool config should look like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. I'll make a note.
- AWS Cognito: The application comes with AWS Cognito enabled by default, so AWS Cognito must be set up before deploying the application. | ||
- Custom domain name with HTTPS support: AWS Cognito requires HTTPS for callback urls to function. | ||
- Access to write to temporary directory: Rails needs to be able to write out temporary files. | ||
- Environment variables: You must provide the application with the environment variables listed in [local.env.example](/app-rails/local.env.example). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want to say more about this specifically for cloud deployments? I know we're setting them as aws secrets and importing them to the ecs containers via the task definition, but if I'm using this repo and not using the infra template, that context would be really helpful.
|
||
## Deploying using the Platform Infrastructure Template | ||
|
||
*Note: The following will be true once https://github.com/navapbc/template-infra/pull/650 is merged.* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels like it belongs in a release note, and not a deployment.md file? Hopefully it won't be an issue for long.
Ticket
Changes
Context for reviewers
Adding pgcrypto as a database migration breaks integration with a more secure and restricted database migration user, such as with the https://github.com/navapbc/template-infra.
Normally, you should NOT delete database migrations. Instead, you should add a follow up migration that reverts the change. However, in this case, if you try to run the application on a more restrictive database migration user, all database migrations from this point on would fail with an error similar to this:
That means a subsequent migration to revert the change would NOT fix the problem.
Release notes
#28 was not included in any releases, but the next release should include a note mentioning the issue and instructing them to run
make db-reset
.Testing
Test local development works in the container
make init-container
make start-container
Test local development works natively
make init-native
make start-native
Test remote deployment using integration with infra template:
validate
branch of https://github.com/navapbc/platform-test-rails