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 bref/symfony-bridge #1129

Merged
4 commits merged into from
Apr 29, 2021
Merged

Conversation

ker0x
Copy link
Contributor

@ker0x ker0x commented Apr 27, 2021

Q A
License MIT

Following #1127, this is the first draft of the recipe.

Ping @t-richard, @mnapoli

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Pull request does not pass validation.

bref/symfony-bridge/0.1/serverless.yaml Outdated Show resolved Hide resolved
bref/symfony-bridge/0.1/serverless.yaml Outdated Show resolved Hide resolved
bref/symfony-bridge/0.1/serverless.yaml Outdated Show resolved Hide resolved
bref/symfony-bridge/0.1/serverless.yaml Outdated Show resolved Hide resolved
bref/symfony-bridge/0.1/serverless.yaml Outdated Show resolved Hide resolved
bref/symfony-bridge/0.1/serverless.yaml Outdated Show resolved Hide resolved
bref/symfony-bridge/0.1/serverless.yaml Outdated Show resolved Hide resolved
bref/symfony-bridge/0.1/serverless.yaml Outdated Show resolved Hide resolved
bref/symfony-bridge/0.1/serverless.yaml Outdated Show resolved Hide resolved
bref/symfony-bridge/0.1/serverless.yaml Outdated Show resolved Hide resolved
@ker0x ker0x force-pushed the bref-symfony-bridge-recipe branch from edb11bd to e4e9abc Compare April 27, 2021 12:21
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Pull request does not pass validation.

bref/symfony-bridge/0.1/serverless.yaml Outdated Show resolved Hide resolved
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Pull request passes validation.

@ker0x ker0x changed the title Add recipe for Bref Symfony Bridge Add recipe for bref/symfony-bridge Apr 27, 2021
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Pull request passes validation.

bref/symfony-bridge/0.1/serverless.yaml Show resolved Hide resolved
bref/symfony-bridge/0.1/serverless.yaml Outdated Show resolved Hide resolved
bref/symfony-bridge/0.1/post-install.txt Show resolved Hide resolved
@@ -0,0 +1,8 @@
{
"copy-from-recipe": {
"serverless.yaml": "serverless.yaml"
Copy link

Choose a reason for hiding this comment

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

Should be serverless.yml

Copy link
Contributor

@t-richard t-richard Apr 27, 2021

Choose a reason for hiding this comment

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

Both are valid for Serverless and the choice to use .yaml may be because of the Validation applied to PRs here

YAML files suffix must be .yaml, not .yml

But the standard for Serverless Framework is indeed .yml. Maybe this could be used as a "workaround"

Suggested change
"serverless.yaml": "serverless.yaml"
"serverless.yml": "serverless.yaml"

Copy link
Contributor

Choose a reason for hiding this comment

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

BTW it's already accepted for docker-compose.yml if that may help and I think we are in the same situation because it's not Symfony config we are talking about but standard in a third-party tool

See for example : https://github.com/symfony/recipes/blob/master/doctrine/doctrine-bundle/1.6/manifest.json#L24

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, i used the .yaml extension to pass validation!

@t-richard wouldn't it be rather

Suggested change
"serverless.yaml": "serverless.yaml"
"serverless.yaml": "serverless.yml"

?

The other solution would be to store the serverless.yml into the package and use copy-from-package instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes it would be "serverless.yaml": "serverless.yml" 🤦

IMHO it makes more sense to have all here

bref/symfony-bridge/0.1/post-install.txt Show resolved Hide resolved
bref/symfony-bridge/0.1/serverless.yaml Outdated Show resolved Hide resolved
bref/symfony-bridge/0.1/serverless.yaml Outdated Show resolved Hide resolved
bref/symfony-bridge/0.1/serverless.yaml Show resolved Hide resolved
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Pull request passes validation.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Pull request passes validation.

Copy link

@mnapoli mnapoli left a comment

Choose a reason for hiding this comment

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

All good to merge for me 👍

Thank you!

@OskarStark OskarStark requested a review from Nyholm April 29, 2021 09:20
handler: public/index.php
timeout: 28 # in seconds (API Gateway has a timeout of 29 seconds)
layers:
- ${bref:layer.php-80-fpm}
Copy link
Contributor

Choose a reason for hiding this comment

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

Based on
https://github.com/brefphp/symfony-bridge/blob/6156395329df3bb34a9a17c5e440ca8ab76c173f/composer.json#L22

you can use the bridge with PHP 7.3, 7.4, but everything in the recipe contains php 8 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Can't see the problem with that 🤔

IMHO We're just providing a decent default (7.3 is EOL and 8.0 is now supported by a fair amount of packages)

If they want, users can replace this line with a previous version this way and it will work

Suggested change
- ${bref:layer.php-80-fpm}
- ${bref:layer.php-74-fpm}

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Pull request passes validation.

Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

Thank you

@ghost ghost merged commit 8e27732 into symfony:master Apr 29, 2021
@t-richard
Copy link
Contributor

Thank you @ker0x 👍

@ker0x ker0x deleted the bref-symfony-bridge-recipe branch April 29, 2021 09:54
This pull request was closed.
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.

5 participants