-
Notifications
You must be signed in to change notification settings - Fork 683
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
add production build setup and aws CICD config files #1087
Conversation
This pull request is automatically deployed with Now. |
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.
Video review with Ron Oribio from the DevOps Platform team
.codebuild/buildspec.deploy.demo.yml
Outdated
commands: | ||
- echo build Docker image on `date` | ||
- docker build -f Dockerfile.prod -t pwa-demo:latest . | ||
- docker tag pwa-demo:latest 276375911640.dkr.ecr.us-east-1.amazonaws.com/pwa-demo |
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.
PR review Ron: add second tag with a version - deploy latest still - use semantic versioning (version file) to keep track of which version is deployed and which was the last deployed.
@@ -0,0 +1,26 @@ | |||
version: 0.2 |
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.
add lifecycle policy for each ECR repo
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.
added ✅
{ | ||
"AWSEBDockerrunVersion": "1", | ||
"Image": { | ||
"Name": "276375911640.dkr.ecr.us-east-1.amazonaws.com/pwa-demo:latest", |
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 tag will need to update based on semantic version tagging
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 is modified in each build script to be either the pr id tag or the commit SHA ✅
dangerfile.js
Outdated
if (typeof stdout !== 'string') { | ||
// execa didn't require | ||
if (err.code === 'MODULE_NOT_FOUND') { | ||
// execa din't require |
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.
typo
docker/.env.docker.prod
Outdated
PORT=8080 | ||
PWA_STUDIO_HOST=localhost | ||
# magento enterprise edition - in production mode | ||
MAGENTO_BACKEND_URL=https://m231-pwa-ent-1.testsonfire.com/ |
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.
pull in from environment variables rather than plain text
docker/.env.docker.prod
Outdated
UPWARD_JS_UPWARD_PATH=venia-upward.yml | ||
UPWARD_JS_BIND_LOCAL=1 | ||
UPWARD_JS_LOG_URL=1 | ||
BRAINTREE_TOKEN=sandbox_8yrzsvtm_s2bg8fs563crhqzk |
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.
token should be pulled in from env vars rather than plain text and other files as well.
packages/venia-concept/package.json
Outdated
@@ -13,7 +13,7 @@ | |||
"venia": "./bin/venia.js" | |||
}, | |||
"scripts": { | |||
"build": "yarn run clean && yarn run build:esm && yarn run validate-queries && yarn run build:prod", |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Assigning @jimbo for review. |
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, so far. 👍
I've left a few questions, but nothing that needs to be addressed.
@@ -19,6 +19,7 @@ | |||
"scripts": { | |||
"build": "yarn workspaces run build", | |||
"build:dev": "yarn workspaces run build:dev", | |||
"ci:pr-checks": "./.bin/pr-checks", |
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.
Would yarn run pr-checks
work here?
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 it would need to be part of node_modules
for it to be accessible directly vs putting it under scripts
.
The only ones I see for execution come up when I run yarn run
:
@sharky.corp.adobe.com ➜ pwa-studio git:(aws-cicd) yarn run
yarn run v1.15.2
info Commands available from binary scripts: JSONStream, acorn, ansi-html, apollo-codegen, atob, autoprefixer, babel, babel-external-helpers, babylon, browserslist, build-storybook, bundlesize, bundlesize-init, bundlesize-pipe, concurrently, conventional-changelog-writer, conventional-commits-parser, conventional-recommended-bump, coveralls, cssesc, danger, danger-ci, danger-init, danger-js, danger-local, danger-pr, danger-process, danger-reset-status, danger-runner, detect, detect-libc, detect-port, disparity, errno, escodegen, esgenerate, eslint, esparse, esvalidate, get-pkg-repo, git-raw-commits, git-semver-tags, gql, graphql, graphql-schema-linter, handlebars, he, html-minifier, hulk, husky-upgrade, import-local-fixture, is-ci, jest, jest-runtime, js-yaml, jsesc, jsome, json5, lerna, loose-envify, miller-rabin, mime, multicast-dns, nearley-railroad, nearley-test, nearley-unparse, nearleyc, needle, node-gyp, node-pre-gyp, nopt, npm-path, npm-run, npm-tree, npm-which, opener, parse-github-url, parse-nodejs-lockfile, parser, pbjs, pbts, pjv, prebuild-install, prettier, prettier-check, rc, react-docgen, regexp-tree, regjsparser, rimraf, run-node, sane, semver, sha.js, shjs, sl-log-transformer, snyk, sshpk-conv, sshpk-sign, sshpk-verify, start-storybook, storybook-server, strip-indent, svgo, swagger-to-graphql, tap-diff, tap-parser, tap-xunit, tape, terser, tree-kill, ts-node, txunit, uglifyjs, upward-js-server, upward-spec, uuid, watch, webpack, webpack-bundle-analyzer, webpack-cli, webpack-dev-server, which, window-size, z-schema, zopfli, zopflipng
} | ||
} | ||
return errors; | ||
}; |
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.
It's not like performance in CI is super important, but could these tasks be run in parallel?
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 that if Danger is going to be used more and expanded then this file should be refactored in a number of ways. I think that should be a separate endeavor outside of this pr though. Mostly what I am doing here is capturing all the errors at once and reporting all the errors, rather than only reporting the first error hit.
RUN yarn install --frozen-lockfile | ||
|
||
# copy over the rest of the package files | ||
COPY packages ./packages |
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.
Can you elaborate on why we can't just copy packages
and run yarn install
?
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.
Copying over just the files needed to install takes advantage of the docker cache, which allows for faster builds when there isn't a change to an image layer, each command in the dockerfile creates a new image layer as it builds. So if no changes are made to the dependencies then those layers will build from cache on subsequent builds. after hte install we copy over the rest of the files and if there are changes to those files then it will rebuild otherwise it will build from cache. More deets on the docs
* add production build setup and aws CICD config files
* add production build setup and aws CICD config files
Description
Adding production build scripts, AWS cicd config files, and pr check scripts for standing up CICD system in AWS.
Related Issue
#1034 , #1012, #1011, #940, #925 , #587
Verification Steps
See CircleCI configuration removed and only test checks performed by AWS in PR #1132