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

Migrate the core of bespin to boto3 #22

Merged
merged 3 commits into from Apr 12, 2017
Merged

Migrate the core of bespin to boto3 #22

merged 3 commits into from Apr 12, 2017

Conversation

atward
Copy link
Collaborator

@atward atward commented Apr 11, 2017

Migrate bespin.amazon.credentials and bespin.amazon.cloudformation to use boto3 (#17). This migrates what I consider the core of bespin (environments & stacks).

Untouched modules (bespin.amazon.ec2, s3, sqs, kms) will continue to operate using the boto authenticator off either the user's environment, or the overlaid assume_role environment.
The migration of the remaining modules will be done in separate PRs and will be tracked in #17.

MVP approach to migrating cloudformation calls to boto3.
More work should be done to migrate to boto3 resources and helpers
(waiters, pagers).

Adds support for update of stack tags
@delfick
Copy link
Owner

delfick commented Apr 11, 2017

cool, thnx, I'll have a proper look a bit later on :)

@atward
Copy link
Collaborator Author

atward commented Apr 12, 2017

@delfick could you sign off on this. There's a number of other PRs hanging off it.

@delfick
Copy link
Owner

delfick commented Apr 12, 2017

ah yes, I'll look now

Copy link
Owner

@delfick delfick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me other than the asserts

self.session = session
if self.session is None:
self.session = boto3.session.Session(region_name=self.region)
assert self.session.region_name == self.region
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a fan of asserts in code.

I'd rather we make session a memoized property than allow it to be passed in

@hp.memoized_property
def session(self):
    return boto3.session.Session(region_name=self.region)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Problem with your example is that is makes a new session for each (CF, EC2, S3...), rather than share the same session.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not a major objection. Pushed new changes.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aaahhh, it is too. I see the difference... hmmmm

Perhaps an object that holds onto a session per region, and we pass that into the intializer, which gets used to generate/reuse a session for that region.....

@@ -48,6 +45,8 @@ def verify_creds(self):
except botocore.exceptions.ClientError as error:
raise BespinError("Couldn't determine what account your credentials are from", error=error.message)

assert self.session is not None and self.session.region_name == self.region
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should raise BespinError instead of an AssertionError

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is more I did something wrong, so changed to ProgrammerError

@delfick
Copy link
Owner

delfick commented Apr 12, 2017

lgtm

@atward atward merged commit 71fc042 into delfick:master Apr 12, 2017
@atward atward deleted the boto3 branch April 12, 2017 06:38
@atward atward mentioned this pull request Apr 18, 2017
atward added a commit to atward/bespin that referenced this pull request Apr 18, 2017
atward added a commit to atward/bespin that referenced this pull request May 9, 2017
Tag modifications are supported since migrating to boto3 in delfick#22
@atward atward mentioned this pull request May 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants