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

[google compute] improved Images coverage #395

Closed
wants to merge 1 commit into
base: trunk
from

Conversation

Projects
None yet
3 participants
@erjohnso
Member

erjohnso commented Nov 20, 2014

Updates to improve the GCENodeImage coverage of the GCE API. Specifically, this update has:

  • Added object attributes to match the upstream Image resource (e.g. diskSizeGb, sourceType, etc)
  • Moving toward deprecating older attributes such as preferredKernel. This has long been removed from the GCE API, but there is a chance very old customer images still retain this value.
  • Add support for setting timestamps for deprecating images for when they would will be automatically marked as DELETED, OBSOLETE, and DEPRECATED.
  • Also adding a nice-to-have feature for allowing users to specify a search-list of projects versus just the user's project and hard-coded list of supported image projects.

No breaking changes introduced and tests have been enhanced for these changes.

if not image:
if (partial_name.startswith('debian') or
partial_name.startswith('backports')):
partial_name.startswith('backports') or
partial_name.startswith('nvme-backports')):

This comment has been minimized.

@sebgoa

sebgoa Nov 21, 2014

Member

@erjohnso just checking that this is not a typo. nvme-backports ? not name-backports ?

@sebgoa

sebgoa Nov 21, 2014

Member

@erjohnso just checking that this is not a typo. nvme-backports ? not name-backports ?

This comment has been minimized.

@erjohnso

erjohnso Nov 21, 2014

Member

@runseb - thanks for checking! Yes, nvme is correct.

@erjohnso

erjohnso Nov 21, 2014

Member

@runseb - thanks for checking! Yes, nvme is correct.

@@ -2218,6 +2246,27 @@ def ex_deprecate_image(self, image, replacement, state=None):
'replacement': replacement.extra['selfLink'],
}
if deprecated is not None:
try:
_ = timestamp_to_datetime(deprecated) # NOQA

This comment has been minimized.

@sebgoa

sebgoa Nov 21, 2014

Member

@Kami quick ping on this , what's our convention for comments at end of a line ?

@sebgoa

sebgoa Nov 21, 2014

Member

@Kami quick ping on this , what's our convention for comments at end of a line ?

This comment has been minimized.

@erjohnso

erjohnso Nov 21, 2014

Member

If it's not clear, I'm ensuring that the string can be properly converted but I don't actually want to use the generated timestamp. The flake8 tests don't like unused variables so I was suppressing it's check on these lines.

@erjohnso

erjohnso Nov 21, 2014

Member

If it's not clear, I'm ensuring that the string can be properly converted but I don't actually want to use the generated timestamp. The flake8 tests don't like unused variables so I was suppressing it's check on these lines.

This comment has been minimized.

@Kami

Kami Dec 11, 2014

Member

(sorry for a late response)

We follow pep8 which says 2 spaces before in-line comment (e.g. bar = 42 # foo)

@Kami

Kami Dec 11, 2014

Member

(sorry for a late response)

We follow pep8 which says 2 spaces before in-line comment (e.g. bar = 42 # foo)

erjohnso added a commit that referenced this pull request Nov 23, 2014

GCE: improved Images coverage
Signed-off-by: Sebastien Goasguen <runseb@gmail.com>

This closes #395

erjohnso added a commit to erjohnso/libcloud that referenced this pull request Dec 1, 2014

GCE: improved Images coverage
Signed-off-by: Sebastien Goasguen <runseb@gmail.com>

This closes #395

@erjohnso erjohnso closed this Dec 10, 2014

@@ -2218,6 +2246,27 @@ def ex_deprecate_image(self, image, replacement, state=None):
'replacement': replacement.extra['selfLink'],
}
if deprecated is not None:

This comment has been minimized.

@Kami

Kami Dec 11, 2014

Member

Sorry for a late comment. Number of duplicated lines of code could be reduced by doing something like that:

for attribute, value in [('deprecated', deprecated), ...]:
   is value is None:
    continue

   try:
    timestamp_to_datetime(value)
   except:
    raise ValueError('deprecated must be an RFC3339 timestamp')

   image_data[name] = value

Anyway, that's just a suggestion and not a priority :)

@Kami

Kami Dec 11, 2014

Member

Sorry for a late comment. Number of duplicated lines of code could be reduced by doing something like that:

for attribute, value in [('deprecated', deprecated), ...]:
   is value is None:
    continue

   try:
    timestamp_to_datetime(value)
   except:
    raise ValueError('deprecated must be an RFC3339 timestamp')

   image_data[name] = value

Anyway, that's just a suggestion and not a priority :)

This comment has been minimized.

@erjohnso

erjohnso Dec 12, 2014

Member

I like it! Adding it to the next PR.

@erjohnso

erjohnso Dec 12, 2014

Member

I like it! Adding it to the next PR.

@erjohnso erjohnso deleted the erjohnso:GCE_Images branch Dec 18, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment