Draft: switch AWS deployment from Serverless to CDK - #121
colorchestra wants to merge 20 commits into
Conversation
✅ Deploy Preview for serene-narwhal-9b7b1c ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for peaceful-raindrop-cb2462 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for serene-narwhal-9b7b1c ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for peaceful-raindrop-cb2462 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
…localhost (cherry picked from commit 69ff6ce)
jh0ker
left a comment
There was a problem hiding this comment.
Claudia und ich haben uns das mal vorgenommen. Alles in allem glaube ich gut. Kannst ja mal über meine Kommentare schauen und überlegen, was davon du dir noch vornehmen möchtest und was wir aufschieben.
| // 403 und 404 auf 200 und "Upps!"-Seite umbiegen; Originalverhalten | ||
| // langfristig vllt. so ändern, dass das nur App-Pfade betrifft und nicht z.B. API-Pfade |
There was a problem hiding this comment.
Ja ich glaub das könnte tatsächlich ein Problem sein. Wir haben einen Service Worker der die Assets cached, wenn wir da einmal ein Asset laden was fehlt, wird er denke ich die "Upps!"-Seite als Response cachen. Das ist dann schwierig, wieder loszuwerden. Ist erstmal kein großes Problem, weil der Offline-Modus hinter einem Feature Flag sitzt, und nur dann auch der Service Worker anspringen sollte.
Aber trotzdem fragwürdig, ob das sinnvoll ist, das jetzt 1:1 zu übernehmen. Wir haben ja jetzt auch z. B. die OAC neu hinzugefügt, also ein 100% "sauberes" Refactoring, wo wir die alten Bugs mitschleppen, ist ja nicht zwingend unser Ziel oder?
| certificate, | ||
| priceClass: cloudfront.PriceClass.PRICE_CLASS_100, | ||
| minimumProtocolVersion: cloudfront.SecurityPolicyProtocol.TLS_V1_2_2021, | ||
| defaultRootObject: 'index.html', |
There was a problem hiding this comment.
Wir haben noch andere Entrypoints, spezifisch /settings/ und /teilen/. Wäre eigentlich besser, wenn dann z. B. /teilen/ auch auf /teilen/index.html pointed. Vorher lief es ja über so einen Website-Endpoint, der das gemacht hat. Sind auch wieder eher so obskure Features, aber die gehen halt kaputt.
vgl. https://programmieren.wdrmaus.de/settings/ vs https://dev.maus.metahost.org/settings/index.html
| concurrency: | ||
| group: deploy-${{ inputs.stage }} | ||
| cancel-in-progress: false | ||
| env: |
There was a problem hiding this comment.
Wir hatten vorher die Environment-Variable BRANCH gesetzt. In webpack.config.js und env.js passieren damit Dinge, und die Piano-Integration nutzt die um zu entscheiden, ob es an den Testing- oder Production-Sink die Events schicken soll.
Ist vielleicht nicht verkehrt, das erstmal zu übernehmen, auch wenn ich da nicht gegen ein Refactoring wäre. In meinem Branch experiment/fix-local-dev-setup habe ich das teilweise schon addressiert (Bucket-Namen werden z. B. nicht mehr über BRANCH abgeleitet). Im Prinzip verbleibt nur noch die Piano-Geschichte und etwas toter Legacy-Code.
| - name: CDK diff → Summary | ||
| working-directory: cdk | ||
| run: | | ||
| npx cdk diff --app cdk.out "MausApp-${{ inputs.stage }}" 2>&1 | tee diff.txt |
There was a problem hiding this comment.
Wenn der cdk diff failed verschluckt die Pipe den exit code und der Step bleibt grün. Ein set -o pipefail würde hier Abhilfe schaffen, alternativ wohl auch ein shell: bash auf dem Step.
| }) | ||
|
|
||
| // Deployment für Assets, die nicht gecachet werden sollen | ||
| new s3deploy.BucketDeployment(this, 'FrontendHtml', { |
There was a problem hiding this comment.
Claudia hat hier ein paar Anmerkungen gemacht, eher nitpicky:
- Warum reicht hier das default
memoryLimitaus, wenn doch beide das selbe Asset-Bundle entpacken? Also zumindest könnte das hier auch eng werden. - Da das
memoryLimitanders ist als das vonFrontendStaticist, werden wohl außerdem zwei Lambda Handlers erzeugt. Wenn man das selbe wählt, erzeugt er nur einen, der dann beide Deploys macht. Wäre also wohl effizienter, das selbe zu wählen.
There was a problem hiding this comment.
Scheint mir teilweise outdated zu sein (Bucket-Namen, DNS setup). Braucht man es noch? DEPLOYMENT.md ist glaub ich näher dran an der Realität.
| // aws-sdk v2 wird gebundlet, weil neuere Node-Runtimes es nicht mehr mitliefern | ||
| // TODO ist auch veraltet, austauschen: https://aws.amazon.com/blogs/developer/announcing-end-of-support-for-aws-sdk-for-javascript-v2/ | ||
| nodeModules: ['aws-sdk', 'nanoid', 'shortid'], | ||
| externalModules: [], |
There was a problem hiding this comment.
Wenn ich das richtig verstehe installisiert er ja damit jetzt ein node_modules in die Lambdas, wo die Backend-Deps komplett drin sind und macht entsprechend auch ein npm ci während des synths.
Braucht es das wirklich, oder kann esbuild das nicht automatisch mit bundlen? Wenn wir in src/backend/lib/s3.js den Import noch anpassen, kann er wohl auch den Großteil droppen und nur den S3-Client mit bundlen, was auch die Bundlesize & cold-starts massiv reduziert.
Wir haben jetzt die Dependencies ja auch dreifach definiert: Hier, in cdk/package.json und der root package.json (auch jeweils mit Lockfile). Wenn wir esbuild alles machen lassen, dann bräuchten wir nur noch die eine im root.
| // TODO ist das sinnvoll? -> evtl auf GET_HEAD umstellen | ||
| // außer wir schreiben direkt so in den Bucket von Cloudfront (aber sollte ja eigentlich die Lambda machen) | ||
| allowedMethods: cloudfront.AllowedMethods.ALLOW_ALL, | ||
| //allowedMethods: cloudfront.AllowedMethods.ALLOW_GET_HEAD, |
There was a problem hiding this comment.
Glaub auch, dass ein ALLOW_GET_HEAD ausreichen müsste.
| prod: 'TODO_PROD_ACCOUNT_ID', | ||
| staging: 'TODO_STAGING_ACCOUNT_ID', | ||
| dev: '391322831368', |
There was a problem hiding this comment.
Scheint ja eher unwahrscheinlich, dass wir noch verschiedene Accounts nutzen werden oder? Kann man von mir aus auch weiter supporten. Die Account-ID könnten wir aber ja schon mal eintragen für alle, oder?
No description provided.