AWS SDK Update and workflow package version update #29
Conversation
There was a problem hiding this comment.
@michaelaguiar So the main point of this PR is the aws/aws-sdk-php package upgrade, right? Can you explain what's the importance of this particular upgrade?
I also see that in config/laravel-echo-api-gateway.php a token property/option dropped out - that's the kind of change which I'm always a bit wary of, could we consider putting that back "just in case"?
Next step is I'd just want to pull this down and install it as part of our app on AWS/Lambda and verify that things are still working ...
|
Probably add a editorconfig file and make sure it's adhered to. Otherwise every maintainer's ide is going to 'fix' code styling on all files. |
@georgeboot Yes I noticed as well that the git diff looks "huge" just because of different indentation/formatting - I might want to have a go at quickly "fixing" that before we merge this to master ... @michaelaguiar I'm gonna change the indentation back to the way it was before, let's try not to mess with formatting when saving a file ... |
Sorry about that! I can go head and get this fixed if you haven't started yet? |
@michaelaguiar Okay yes please do, thank you! |
|
Ok just fixed the formatting. Looks like most of the diff now is I also agree with the editorconfig addition to enforce syntax styles. |
|
@michaelaguiar I'm now checking my own fork (because I'm seeing that for our app we're still using our fork, not the main repo) and I see that in the past I also tried to upgrade I tested that and it didn't work, so I reverted both changes (the SDK upgrade and also the token deletion) ... but:
So I'm just gonna test it with this PR, and if it works for our app (with PHP 8) then we can switch our app to PHP 8, and from the fork to the main repo - that would definitely be progress! |
|
Yea my deployments were failing because the newer AWS SDK version expected an array instead of a string for token if I remember correctly. Sounds good. I believe I pulled over all changes to fix it, but let me know if you run into any issues. |
@michaelaguiar @georgeboot Woohoo, lo and behold it works, victory! I pulled the PR down, messed a bit with our composer.json to load the dependency locally, then did an AWS/Bref deployment:
So we need to delete that "token" again (I'll do that) and then I'm gonna merge this PR ! |
@michaelaguiar No wait lol, seems I can't make this change, because the PR branch to be merged into master is within your forked repo, can't modify it ... :) Can you make that change please, i.e. delete the line with "token" again from P.S. websockets absolutely working like a CHARM - and all of that on top of the Bref PHP 8.1 runtime and new PHP AWS SDK ! |
|
Done! I have one other contributor that has added some new functionality on my fork. I am going to review it and see if we want to get it merged in this repo. |
@michaelaguiar Thanks, I've merged the PR now! Yeah if the change from that collaborator looks useful then just create a PR for it and we'll check it out ... @georgeboot Just a (probably) "dumb" question: we've now merged this PR, but do we need to increase the composer version somewhere so that people (e.g. myself, or Michael) can pull a new version of this dependency? I just assume we need to increase a version number somewhere but I'm not sure lol ... :) |
|
You need to release a new version from GitHub ;-) |
@georgeboot Ah right, that makes sense, of course this stuff needs to go somewhere in the Composer "registry" ... you want to do that, or you want me to attempt that? By the way I saw that a couple of CI jobs were failing now, not sure if that's serious or not ... |
|
If you release on GitHub it will automatically sync to packagist. I'll try to take a look at the CI jobs for this time. |
@georgeboot Thanks, that might be helpful, I didn't really look at it yet, just noticed that there were errors ... can look at it next time but I'm off to bed now :) P.S. message is pretty clear, it's trying to run "pest" but that depends on a Composer plugin which is currentlty 'blocked' - question is of course if something in the PR caused this, or it was already like this before the PR :) |
@michaelaguiar @georgeboot Fixed it, at least the "pest" run - AWS-SDK-PHP wants PHP 8.1 minimum, so I increased the PHP version in ci.yml - but Larastan still fails, we're using an old version which doesn't support 8.1 - I tried to upgrade it but that wasn't successful ... i'll continue with that - will fix the Larastan issues and then publish to Packagist :) |
|
@michaelaguiar @georgeboot Released it to Packagist now - version 0.5.0 ! Did a quick sanity test with our app, looks good - we can now say goodbye our fork and use "the real thing" ... (however - I saw a build error notification saying that the NPM package couldn't be published - but, the existing version 0.1.2 is still there - https://www.npmjs.com/package/laravel-echo-api-gateway ... will look at this "later") P.S. have also got Larastan running now - it's flagging a number of issues such as declaring parameters |
|
Awesome, thanks for getting that merged! When I have some time, I can take a Quick Look at the larastan warnings and see if I can quickly get them updated as well. Sounds like minor syntax warnings so far. |
No description provided.