-
-
Notifications
You must be signed in to change notification settings - Fork 476
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 recipe for runtime/bref #1001
Conversation
Thanks for the PR 😍 Diff between recipe versionsIn order to help with the review stage, I'm in charge of computing the diff between the various versions of patched recipes. |
Co-authored-by: Fabien Potencier <[email protected]>
Oops, sorry, Updated now. |
# Read the documentation at https://www.serverless.com/framework/docs/providers/aws/guide/serverless.yml/ | ||
service: my-symfony-app | ||
configValidationMode: error | ||
useDotenv: true |
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.
Unless I'm missing something you may not want to use useDotenv: true
.
This will inject all .env
env variables (application variables) into the serverless
process (build/deploy process). And it will also automatically exclude .env
files from being uploaded.
This feature was supposed to be enabled by default in Serverless Framework v3, but we've decided to keep it opt-in to avoid messing up applications. At the moment the error message says that this line should be added, but this will be changed soon.
If you want to control .env
files separately from serverless deploy
then you don't want to mix the 2.
# Excluded files and folders for deployment | ||
- '!assets/**' | ||
- '!node_modules/**' | ||
- '!public/build/**' |
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 think we should exclude the whole public/build
directory (even if you add entrypoints.json
and manifest.json
files back).
What happens to files bundled by webpack Encore? They won't be deployed with this configuration 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.
These assets should be served through s3 or Cloudfront. Serving them from lambda is highly inefficient in terms of performance or costs. See https://bref.sh/docs/frameworks/symfony.html#assets and https://bref.sh/docs/websites.html
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.
Oh good point, thx!
stage: prod | ||
runtime: provided.al2 | ||
environment: | ||
# Symfony environment variables | ||
APP_ENV: prod |
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.
stage: prod | |
runtime: provided.al2 | |
environment: | |
# Symfony environment variables | |
APP_ENV: prod | |
stage: ${opt:stage,'prod'} | |
runtime: provided.al2 | |
environment: | |
# Symfony environment variables | |
APP_ENV: ${self:provider.stage} |
By doing this, the user can easily deploy a prod/env Symfony app by specifying --stage
when deploying.
At work, we used to work on multiples stages/Symfony environments:
- a "staging" environement: we automatically deploy branch
develop
or pull requests with--stage dev
, which will run a Symfony app indev
environment - a "production" environment: we automatically deploy branch
main
with--stage prod
, which will run our Symfony app inprod
environment
What do you think?
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 not needed if you use the variable ${sls:stage}
which looks first for the cli option first, then the provider.stage value then defaults to dev if none is found.
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.
+1 this is not needed, you should consider stage
like a defaultStage
configuration (we are considering renaming it to that in the future).
Also you might not want to mix Symfony env and stages, as you can have many stages (e.g. one per developer) which would not map to Symfony environments.
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.
Definitely agree here, those are two different concepts that I would not mix to avoid confusion.
Also see my previous comment here about this #1001 (comment)
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.
Alright, didn't know about that, thanks!
Friendly ping @Nyholm, just in case you forgot about it :) |
Reading symfony/recipes-contrib#1283, it looks like we should close both. |
https://github.com/php-runtime/bref