-
Notifications
You must be signed in to change notification settings - Fork 8
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 support for passing an AWS.Credentials object #8
base: master
Are you sure you want to change the base?
Conversation
I like the idea of supporting any credentials system, it's not the first time a patch about that issue pops up in this repo. I think its worth reworking how we handle credentials now. Looking at the credentials object seems like it would be a better solution to move Some examples of what I'm talking about: How is used currently:// ...
gulpEbDeploy({
// ...
amazon: {
accessKeyId: "abcdefg", // optional
secretAccessKey: "QWERTYASDFGH12345", // optional
// ...
},
}) How could be used after this patch:Example 1Same params as before, different object strcuture gulpEbDeploy({
// ...
amazon: {
credentials: {
accessKeyId: "abcdefg", // optional
secretAccessKey: "QWERTYASDFGH12345", // optional
},
// ...
}
}) Example 2Same params as before, AWS CLI credentials will be used gulpEbDeploy({
// ...
amazon: {
// No credentials object passed.
}
}) Example 3New Allow any kind of object strcture under credentials, gulpEbDeploy({
// ...
amazon: {
credentials: { /* .... */ },
// ...
},
}) Example 4New Allow gulpEbDeploy({
// ...
amazon: {
credentials: new AWS.Credentials({ /* .... */ }),
// ...
},
}) In order to keep this change a minor version and not a major, we should keep support for both approaches and maybe show a warning in case you are using the old system. |
What I'm trying to say with the previous comment is, are you up to implementing something like that? If not i think I can revisit this on my weekend and give it a shot. |
@Souler Example 2 and 4 are already supported in this PR. Wrapping AWS.Credentials construction as laid out in example 3 actually adds complexity on both ends. First there's error handling/rethrowing as you already mentioned on the ebdeploy side and catching as well as handling the error on the integration side. tl;dr my vote is fully support 2, 4 and also keep 1 around, but mark it deprecated. |
The reason behind the point 3 is being able to keep the config passed to the plugin a plain object. This allows to store the config on a json file (or any other plain text filetype) and load it on build time, which a lot of times is desireable too. Maybe, pretending to handle any errors thrown by Also, on the example 1 topic, I would add a log yellow warning with the deprecation notice. My bottom line: also support example 3 (but dont overkill it), show a deprecation warning when using example 1. |
b26aa27
to
5cc7697
Compare
Just a heads up, I've updated the PR to support all the requested cases. It's not ready to merge and almost completely untested. I'm still not convinced supporting Example 3 since it's a maintenance liability, takes some semi-educated guesses and then still fails to support all credential providers. |
5cc7697
to
5bf55fe
Compare
c69d132
to
736d870
Compare
e5469e7
to
b5ad087
Compare
b5ad087
to
d156f6e
Compare
d156f6e
to
3e755a2
Compare
2 similar comments
…eVersion) via amazon.config useful for setting up proxies, loggers, retry parameters, etc
1 similar comment
…lk env otherwise the deployment will fail
1 similar comment
@Souler This should now be ready to merge. I've also added two more changes:
|
@Souler any chance to get this merged |
This PR adds a
credentials
field tooptions.amazon
which takes an AWS.Credentials object and assigns it toAWS.config.credentials
.I'm aware that gulp-elasticbeanstalk-deploy already supports
accessKeyId
andsecretAccessKey
, however these alone only work for permanent credentials. Adding support for AWS.Credentials should make this module work in pretty much all cases including AssumeRole accounts and federated users.