-
Notifications
You must be signed in to change notification settings - Fork 4
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
[SWP-5969] - Allow missing credentials to make use of injected IAM role credentials #65
Conversation
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.
This looks good! I will not request changes here but rather comment as PHP is not my field of expertise.
/** | ||
* Make sure some AWS credentials were provided to the configuration array. | ||
* | ||
* @return bool |
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.
In this JavaDoc string, can you type the input argument as well?
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.
It's PHPDoc, which I guess is pretty much the same. We could indeed add @param array $config
- for consistency, but believe we just took the code that was added as part of the PR to bring this functionality into the 0.4
branch.
Could we also bring in over the EventBridgeBroadcasterTest and SnsBroadcasterTest?
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.
Good stuff. Only thing for me is also bringing the tests over that cover the introduced functionality, and then also a little into look into why we now need --no-scripts
.
/** | ||
* Make sure some AWS credentials were provided to the configuration array. | ||
* | ||
* @return bool |
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.
It's PHPDoc, which I guess is pretty much the same. We could indeed add @param array $config
- for consistency, but believe we just took the code that was added as part of the PR to bring this functionality into the 0.4
branch.
Could we also bring in over the EventBridgeBroadcasterTest and SnsBroadcasterTest?
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.
Great stuff 🚢
Let's double check it works as expected with PP and then we can do the patch release 🚀
This is to support the migration from IAM users to IAM roles for applications running earlier versions of Laravel (< 8).