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
Travis cd #34
Travis cd #34
Conversation
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.
Looks good to me, I think all the changes I have comments about are intentional but I've added some comments just to double check
config/development.env
Outdated
NODE_ENV=production | ||
OAUTH_CLIENT_ID=AQECAHjqALewp8JBJNxIQvR4oY795dyG7INaGR1glMsTEgetggAAAHMwcQYJKoZIhvcNAQcGoGQwYgIBADBdBgkqhkiG9w0BBwEwHgYJYIZIAWUDBAEuMBEEDGM1ccp/vwImDcIFwQIBEIAwl1AV5yfZ/tquR4OY9w48ImVdZNi3wXluoY6LqyMDB5s1scHksWy1dmtZLjXAhRi8 | ||
OAUTH_PROVIDER_URL=https://isso.nypl.org/oauth/token | ||
NYPL_DATA_API_URL=https://platform.nypl.org/api/v0.1/ |
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.
Are we sure NYPL_DATA_API_URL is just for getting info?
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 believe this service does look ups for patron and item record information to pass along to ReCAP's API. For development environments, the URL should be https://dev-platform.nypl.org/api/v0.1.
@@ -0,0 +1,13 @@ | |||
NODE_ENV=production |
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.
Is NODE_ENV=production right?
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. All deployed code in any environment should operate as NODE_ENV=production. The only place that sometimes changes is when running tests (sometimes convenient to set NODE_ENV=test) or when running locally. NODE_ENV has little to do with deployment environment, which confusingly uses similar terms.
"run-local": "./node_modules/.bin/node-lambda run -f ./config/local.env -j event.json", | ||
"deploy-development": "./node_modules/.bin/node-lambda deploy -e development -f ./config/development.env -S config/event_sources_development.json --role arn:aws:iam::224280085904:role/lambda_basic_execution --profile nypl-sandbox", | ||
"deploy-qa": "./node_modules/.bin/node-lambda deploy -e qa -f ./config/qa.env -S config/event_sources_qa.json --role arn:aws:iam::946183545209:role/lambda-full-access --profile nypl-digital-dev", | ||
"deploy-production": "./node_modules/.bin/node-lambda deploy -e production -f ./config/production.env -S config/event_sources_production.json --role arn:aws:iam::946183545209:role/lambda-full-access --profile nypl-digital-dev", |
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.
No VPC?
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.
No. I believe there's no reason to put a service that doesn't connect to a database inside a VPC.
config/qa.env
Outdated
NODE_ENV=production | ||
OAUTH_CLIENT_ID=AQECAHh7ea2tyZ6phZgT4B9BDKwguhlFtRC6hgt+7HbmeFsrsgAAAHMwcQYJKoZIhvcNAQcGoGQwYgIBADBdBgkqhkiG9w0BBwEwHgYJYIZIAWUDBAEuMBEEDGjgjo/vQ4NAC/lq9gIBEIAwI7RrzwumIX3u0I0QD4LO9z8uhtgIuU75+FH5XeQl0xE42HIm2n8Mn1rkXry6TR0N | ||
OAUTH_PROVIDER_URL=https://isso.nypl.org/oauth/token | ||
NYPL_DATA_API_URL=https://platform.nypl.org/api/v0.1/ |
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 should also point to https://dev-platform.nypl.org/api/v0.1/
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.
you mean qa-platform.nypl..
right?
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 -- am still living in "we don't have qa set up land."
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.
Minor question about config file
config/development.env
Outdated
NODE_ENV=production | ||
OAUTH_CLIENT_ID=AQECAHjqALewp8JBJNxIQvR4oY795dyG7INaGR1glMsTEgetggAAAHMwcQYJKoZIhvcNAQcGoGQwYgIBADBdBgkqhkiG9w0BBwEwHgYJYIZIAWUDBAEuMBEEDGM1ccp/vwImDcIFwQIBEIAwl1AV5yfZ/tquR4OY9w48ImVdZNi3wXluoY6LqyMDB5s1scHksWy1dmtZLjXAhRi8 | ||
OAUTH_PROVIDER_URL=https://isso.nypl.org/oauth/token | ||
NYPL_DATA_API_URL=https://qa-platform.nypl.org/api/v0.1/ |
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.
shouldn't this be dev-platform?
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.
Seems good to me
I believe all requested changes have been made [?]. Thanks for taking a look. |
This PR:
deploy-[env]
command can be used by Travis for CD