-
-
Notifications
You must be signed in to change notification settings - Fork 365
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
0.3 laravel #223
0.3 laravel #223
Conversation
Gimme a day or so to fix the Code Formatting and Travis errors. |
Oh, this looks nice. We can probably also incorporate something similar for Symfony with the |
Yes. Artisan is built on the Symfony console package. Once I get the Laravel bridge sorted and merged, I can begin looking at refactoring the Symfony bridge to share the logic. |
(collect($methods))->each(function (string $method) use ($route, &$samConfig) { | ||
list($name, $config) = $this->routing($method, $route->uri, $route->getName()); | ||
$this->info('Setting: ' . $name); | ||
$samConfig['Resources']['Website']['Properties']['Events'][$name] = $config; |
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.
Is this necessary because of the SAM local's problem with catch-all routing? (see aws/aws-sam-cli#437)
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.
Yes, that is what drove me to it. But then there is also the fact that by not having a catch-all route, we don't have to pay Lambda costs for bots that are attempting to search through URLs that are no good, because the API Gateway will simply block those for us.
I believe that we can make it even more mature over time. But for now, it seemed the best solution.
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 was tempted to do something similar, and I like your rationalisation about the costs for non-existent routes! However, does that mean that your application won't be able to supply custom 404 pages to the user?
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.
The short answer is yes, that is what it means. The longer answer is that we can customize API Gateway error responses.
Or, we could simply change the route configuration to always add the catch-all route as well as defined routes. Then, when running on AWS, the catch-all will do it. I think longer term, I would personally prefer to limit the vulnerability by having API Gateway block those.
What do you reckon?
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 think it depends on the application you're running on Lambda, and how important the 404 entry in the application log is to you / your users.
If I'm serving a REST API, for example, then I don't care what the user sees (as long as the 404
status code is correct), and those logs probably aren't that important compared to other entries in the application log. In this case, I'd also definitely appreciate API Gateway rejecting 'bad' traffic.
Serving a standard website, however, is a different matter. I want to show the user a nice 404 page with links to help them find their way, and I'll probably want to have the information go to Loggly or something so I can detect and diagnose broken routes in my system.
Perhaps this needs to be an optional configuration setting the user can choose?
(I have to admit, I haven't done custom API Gateway error responses, so maybe my argument here is invalid)
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 will just add the catchall as a default route for now. We can refine it later.
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.
@nealio82 If you want to jump on the Bref Slack to discuss things ... https://join.slack.com/t/brefworkspace/shared_invite/enQtNTQyNTQ2NDM4NTgzLTBkYTYyNGEyYWIxNzE4NmM5N2QyM2E1OTI3NTM0NDdhZDE2YzI2MGZjNDRmNDMzNzBmMmJiZjVlY2Q1Yjg4NDg come on!
Hi, nice work! Given the size of this PR I'll try to properly review and write up my comments during the week. |
@mnapoli would you prefer to make a branch and have me delete this and provide another pull request against said branch? |
### Local API Testing | ||
If you have docker installed, and would like to test your code locally. This command will run your code in docker and give you a local testpoint to check it out in. | ||
``` | ||
artisan bref:start-api |
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.
just a thought: i wonder if this might be better as artisan bref:start-app
instead. we know that under the hood it's calling sam local start-api
, but as far as the end user is concerned it might be confusing to type start-api
if they're not creating a rest api..?
I've tried to note all the changes in this pull request to identify what to do with each:
That is interesting and ideally we should aim to support Lumen. Given the size of the PR I don't know if it is easy to do separately from that and what it actually implies.
I don't think we should add Laravel-specific commands for something that is global to Bref. Now the question is: should we wrap SAM into Bref commands. After Bref 0.2 I'm not convinced we should anymore, at least not if we have strong reasons. This is still an interesting discussions, and I have several ideas which could constitute good reasons for doing this. In any case in this PR are there strong reasons to wrap SAM commands? For the record I can detail why I think we should try to avoid this, but there are many things to review here (and it's late) so I won't go into those details now.
I love that idea!
Love that!
Love that as well, that will solve a very painful issue. I think this is acceptable to force that config because it's a low-level/internal config value.
While I see the reason, I'm not a huge fan of silently overwriting the user's configuration. Maybe it would make more sense to turn that into validation: if the session is configured to go to disk, throw an exception. That will get their attention. Yes that means that users have to spend some time configuring Laravel for Lambda, but ideally we are talking about 2-3 env variables. That's good, we should not hide too much, as long as it's simple to do and maintain. It's good that users understand why sessions (and logs) should not be written to disk. If they don't spend 30 seconds understanding that difference about Lambda they will have a lot of surprises along the way.
Same as with the session, ideally it should be a matter of simply setting one environment variable and that's it. We could add that to the validation phase as well. And we could keep your idea of the
This is a lot of added complexity, both to how Bref works and to using Bref. Now we have another piece of orchestration on top of SAM. Ideally I'd want to avoid that. (an alternative would be to wrap and hide sam entirely but this is a big job and that means isolating Bref from SAM's new features, community, documentation, etc.) An alternative to this could be to start using parameters and variables in In any case given the size of the change I'm not sure I understood all the advantages the approach in this PR brings. Could you explain what we gain? That way we can see if there are other solutions to consider.
I'm not sure I understand the reasons for this, could you detail them?
From what I've seen this isn't necessary as the
This is interesting but this isn't specific to Laravel, and this is not needed when using Bref with a "proper" CI/CD pipeline. Unless I'm mistaken this is useful when deploying from a local dev installation. Wrapping SAM to control the archive process is a bit of work and yet another piece of complexity I'd try to avoid. Maybe we could take the problem differently and look at how other deployment systems/hosting platform work. That's what I did when I decided to remove
|
Perhaps, before we go through all of the details, it is more apt to determine if bref wants to provide this degree of support for framework bridges. It will necessarily add a lot more complexity, and if we desire bref to stick to simply providing the runtime, perhaps it makes more sense for us to have a separate package that handles this degree of integration? Not only do separate packages reduce complexity for bref, it can also spread out support. If @nealio82 and I are interested in pursuing a Symfony/Laravel specific bridge, we go off and do that ... along with having to bear the consequences of support ourselves. :-) However, if we do want to include this degree of framework bridging in bref itself, I will go through the points and address them from my perspective. |
I think Laravel / Symfony bridge projects would be a good way of removing some implementation-specific concerns from the code (plus a lot of dependencies!). @mnapoli would then be free to concentrate on the core project while other contributors can donate code based upon their own use-cases (Lumen, Laravel, Symfony, Zend, etc) without muddying the water for every other possible user. If we set up a Github org, in my mind we could have |
I like this idea. Breaking the bits out into specialized repos/smaller projects should help move the entire project forward. However, there is also a concern around dedication to the project and how long people who are excited about it today will stick around to continue supporting/growing it. Let's take that conversation to Slack, where we can be a bit more dynamic? |
I'm doing a bit of cleaning in the issue tracker, closing this as it's stale and there are ideas to implement in smaller bits. There is also https://github.com/stechstudio/laravel-bref-bridge for those interested in the approach here. |
Updates the Laravel bridge to allow all the BREF work to be accomplished via Artisan. The goal is make the Laravel bridge work as simply and natively as possible for Laravel Developers that desire to leverage Lambda.
Given a new Laravel project, the following should handle everything required to publish the porject to Lambda.
artisan bref:start-api
is useful for testing locally in docker.artisan bref:update
is useful for simply pushing code changes to Lambda without dealing with Cloudformation via theartisan bref:deploy
command.I have also included the Documentation Update for Laravel.