Skip to content
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

Closed
wants to merge 5 commits into from
Closed

Add recipe for runtime/bref #1001

wants to merge 5 commits into from

Conversation

Nyholm
Copy link
Member

@Nyholm Nyholm commented Sep 19, 2021

@github-actions
Copy link

github-actions bot commented Sep 19, 2021

Thanks for the PR 😍

Diff between recipe versions

In order to help with the review stage, I'm in charge of computing the diff between the various versions of patched recipes.
I'm going keep this comment up to date with any updates of the attached patch.

runtime/bref/0.2/serverless.yaml Show resolved Hide resolved
runtime/bref/0.2/serverless.yaml Outdated Show resolved Hide resolved
runtime/bref/0.2/serverless.yaml Show resolved Hide resolved
runtime/bref/0.2/serverless.yaml Outdated Show resolved Hide resolved
Co-authored-by: Fabien Potencier <fabien@potencier.org>
runtime/bref/0.2/bootstrap Outdated Show resolved Hide resolved
@Nyholm
Copy link
Member Author

Nyholm commented Sep 20, 2021

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
Copy link

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/**'
Copy link
Contributor

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?

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh good point, thx!

Comment on lines +11 to +15
stage: prod
runtime: provided.al2
environment:
# Symfony environment variables
APP_ENV: prod
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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 in dev environment
  • a "production" environment: we automatically deploy branch main with --stage prod, which will run our Symfony app in prod environment

What do you think?

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.

Copy link

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.

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)

Copy link
Contributor

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!

@nicolas-grekas
Copy link
Member

Friendly ping @Nyholm, just in case you forgot about it :)

@fabpot
Copy link
Member

fabpot commented Dec 17, 2022

Reading symfony/recipes-contrib#1283, it looks like we should close both.
Closing now, feel free to reopen if you changed your mind.

@fabpot fabpot closed this Dec 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants