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

allow using a pre-configured IAM role #62

Closed
wants to merge 33 commits into from
Closed

allow using a pre-configured IAM role #62

wants to merge 33 commits into from

Conversation

sdole
Copy link

@sdole sdole commented Jul 21, 2016

see issue #61. This is my proposal to fix it.

@jamesls
Copy link
Member

jamesls commented Jul 28, 2016

Sorry for the delay, I've been OOTO until today.

Overall, I think this change looks good. I do have one question. In considering additional features such as #40 where I'd like to track everything I create so that I can subsequently delete them in a chalice delete command, I'm thinking about writing to disk every thing I create, possibly in the config.json. If that's the case, it might help if the config was more explicit about whether or not to create a role and autogenerate a policy. What do you think about something like this:

{"role_arn": "arn:....", "manage_role": false}

Not really sure what to call the config option, but some sort of boolean config option that explicitly says "don't touch the role_arn".

What do you think?

@sdole
Copy link
Author

sdole commented Jul 29, 2016

good idea! done. also, changed the config keys to iam_role_arn and manage_iam_role so that it is clear we are talking about iam roles. thanks!

@sdole
Copy link
Author

sdole commented Aug 1, 2016

Hello, Let me know if you want me to make any changes. I am fairly new to python, so, don't hesitate to comment on my code - i will learn something. thanks.

@sdole
Copy link
Author

sdole commented Aug 1, 2016

this build seems to have failed due to some documentation changes unrelated to this pull request.

@jamesls
Copy link
Member

jamesls commented Aug 1, 2016

Thanks. Would you mind squashing your commits and rebasing against master? That should fix the doc errors you're seeing.

Sachin Dole and others added 2 commits August 1, 2016 19:41
if an IAM role is configured, use that instead of creating new

how to use a pre-configured IAM role instead of creating new

added boolean config param manage_iam_role to explicitly turn iam on or off

added boolean config param manage_iam_role to explicitly turn iam on or off

update doc to show new keys for managing iam role

broke a too long line into two lines

removed formatting changes my commit added to other people's code

if an IAM role is configured, use that instead of creating new

added boolean config param manage_iam_role to explicitly turn iam on or off

if an IAM role is configured, use that instead of creating new

update doc to show new keys for managing iam role

broke a too long line into two lines

removed formatting changes my commit added to other people's code

fixed typo!
@sdole
Copy link
Author

sdole commented Aug 2, 2016

squashing and rebase was getting complicated. I just re-did a new fork, manually applied changes and requested a new pull #85. Closing this.

@sdole sdole closed this Aug 2, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants