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

add ex_encrypted and ex_kms_key_id kwargs in EC2NodeDriver.create_volume #729

Closed
wants to merge 3 commits into from

Conversation

ViktorOgnev
Copy link
Contributor

@ViktorOgnev ViktorOgnev commented Mar 28, 2016

No description provided.

@@ -3129,6 +3139,12 @@ def create_volume(self, size, name, location=None, snapshot=None,
if ex_volume_type == 'io1' and ex_iops:
params['Iops'] = ex_iops

if ex_encrypted:
Copy link
Contributor

@tonybaloney tonybaloney Mar 28, 2016

Choose a reason for hiding this comment

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

can you make value checks more explicit please e.g. if ex_encrypted is not None:

@tonybaloney
Copy link
Contributor

tonybaloney commented Mar 28, 2016

hi @ViktorOgnev thanks for your contribution, there are only a couple of changes before we can merge, first I made a comment about explicit "truthy" checking, can you fix that on both lines please. Second the lint is failing because 3 of the lines are too long, probably the docstring comments from a glance. you can run flake8 locally to check the next commit.

@ViktorOgnev
Copy link
Contributor Author

ViktorOgnev commented Mar 28, 2016

hi @tonybaloney , looks like now it's ok) Should I rebase, or is it fine as is?

@tonybaloney
Copy link
Contributor

tonybaloney commented Mar 28, 2016

perfect, no need to squash, will merge.

@asfgit asfgit closed this in 7dc0fb0 Mar 28, 2016
@tonybaloney
Copy link
Contributor

tonybaloney commented Mar 28, 2016

all done, thanks for your contribution @ViktorOgnev

asfgit pushed a commit that referenced this issue Mar 28, 2016
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