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.

Sign up for GitHub

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

WIP: Use buildDir for the bridge #36

Closed
wants to merge 2 commits into from
Closed

WIP: Use buildDir for the bridge #36

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c95a35b
Tweak README.md
t-richard Mar 30, 2021
b30cb1e
Throw exception if no buildDir in the deployed package
t-richard Mar 31, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,9 @@ composer req bref/symfony-bridge

## Usage

You only need to one one small change To quickly setup Symfony to work with Bref.
You only need to do two small change to quickly setup Symfony to work with Bref.

First change your `src/Kernel.php` like so :

```diff
// src/Kernel.php
Expand All @@ -33,6 +35,19 @@ use Symfony\Component\Routing\RouteCollectionBuilder;
// ...
```

Then make sure your `serverless.yaml` contains at least the following include/exclude rules

```yaml
# ...
package:
exclude:
- var/**
include:
- var/cache/prod/**
- var/build/prod/**
mnapoli marked this conversation as resolved.
Show resolved Hide resolved
#...
```

Now you are up and running.

## Optimize first request
Expand Down
28 changes: 28 additions & 0 deletions src/BrefKernel.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,11 @@ public function isLambda(): bool
return getenv('LAMBDA_TASK_ROOT') !== false;
}

public function allowEmptyBuildDir()
{
return false;
}

/**
* {@inheritDoc}
*/
Expand All @@ -25,6 +30,29 @@ public function getCacheDir()
return parent::getCacheDir();
}

/**
* {@inheritdoc}
*/
public function getBuildDir(): string
{
$buildDir = $this->getProjectDir().'/var/build/'.$this->environment;

if ($this->isLambda() && !is_dir($buildDir)) {
if (!$this->allowEmptyBuildDir()) {
throw new \Exception(
'You must warm and deploy the build directory as part of your Lambda package
as this directory is readonly on Lambda.
You can alternatively define the function allowEmptyBuildDir() in your Kernel class
and return "true" if you still want to deploy without a warm build directory.
');
Copy link
Member

Choose a reason for hiding this comment

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

If we return true, the app will break right? (because the build directory is read-only, it cannot be warmed)

Copy link
Member Author

Choose a reason for hiding this comment

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

If we return true, the exception is not thrown so it continues to line 49 return $this->getCacheDir(); which will eventually resolve to /tmp/cache so it will not error but warm the cache and build at runtime on each new lambda.

Copy link
Member Author

Choose a reason for hiding this comment

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

This allows for people that don't care about performance or just want to quickly test without bothering on deploying a properly warmed cache to do it.
But as it's not the recommend approach I prefer it to be false by default and throw an explicit exception.

This would also need to be documented and is just a proposal, not a definitive opinion of mine.

Copy link
Member

Choose a reason for hiding this comment

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

🤦 you're right, no exception/error here.

Personally, I'm a big fan of being able to deploy without any step (i.e. without cache generation). It's important for the DX, especially when testing out.

But because of that preference, I've slowed the effort of other contributors in the past. Now I think it's more important to get it working well for Symfony 5.2 in good condition, and maybe later iterate on that and allow deploying without cache.

Copy link
Member Author

@t-richard t-richard Mar 31, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

No worries, I think thats good discussions to have but I agree that we can iterate and improve DX later on or support more use cases.

}

return $this->getCacheDir();
}

return $buildDir;
}

/**
* {@inheritDoc}
*/
Expand Down