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

first step towards image refactoring #495

Merged
6 commits merged into from
Dec 4, 2015
Merged

Conversation

glyph
Copy link
Contributor

@glyph glyph commented Nov 30, 2015

Begin to address #492 - this is just an initial example of how to use a plain-vanilla Image class rather than a subclass; there's lots more refactoring work to do, but this is a proof of concept.

Review on Reviewable

Image.image_id and Image.image_size are both classmethods that conflict
with attribute names; the classmethods generate random IDs but the
attributes are just values.  Move them out to functions.
@codecov-io
Copy link

Current coverage is 99.22%

Merging #495 into master will not affect coverage as of 45fffd2

@@            master    #495   diff @@
======================================
  Files           73      73       
  Stmts         4281    4283     +2
  Branches       636     636       
  Methods          0       0       
======================================
+ Hit           4248    4250     +2
  Partial         23      23       
  Missed          10      10       

Review entire Coverage Diff as of 45fffd2

Powered by Codecov. Updated on successful CI builds.

@glyph
Copy link
Contributor Author

glyph commented Dec 3, 2015

Hey @gabecase - we're pretty light on code reviewers right now, do you think you could have a look?

@ghost
Copy link

ghost commented Dec 4, 2015

@glyph sure I can look at this some tonight and tomorrow morning, will put some comments here on anything I find

@lekhajee
Copy link
Contributor

lekhajee commented Dec 4, 2015

+1 lgtm. Sorry it took so long.

@lekhajee
Copy link
Contributor

lekhajee commented Dec 4, 2015

@gabecase: feel free to merge the PR if you don't have any comments.

@ghost
Copy link

ghost commented Dec 4, 2015

Ok awesome 2 sets of eyes even better :) checking this out now

"Fedora 22 (PVHVM)": {"minRam": 512, "minDisk": 20,
"OS-EXT-IMG-SIZE:size": Image.image_size(),
"OS-EXT-IMG-SIZE:size": random_image_size(),
Copy link

Choose a reason for hiding this comment

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

👍 I like the re-useable random_image methods for the test data

@ghost
Copy link

ghost commented Dec 4, 2015

LGTM!

ghost pushed a commit that referenced this pull request Dec 4, 2015
first step towards image refactoring
@ghost ghost merged commit ac47e54 into rackerlabs:master Dec 4, 2015
@glyph glyph deleted the image-refactor branch December 4, 2015 06:52
This pull request was closed.
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.

3 participants