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

WIP: Generalize building of artifacts using classes #946

Draft
wants to merge 1 commit into
base: master
from

Conversation

@darkmuggle
Copy link
Contributor

darkmuggle commented Nov 22, 2019

This folds a lot of the copy-pasted buildextend code into a class.

@darkmuggle darkmuggle requested a review from ashcrow Nov 22, 2019
raise NotImplementedError(f"target {imgtype} has not been ported yet")

log.info(f"Loading Image Class for {mod_name}")
mod = pydoc.locate(mod_name)

This comment has been minimized.

Copy link
@ashcrow

ashcrow Nov 22, 2019

Member

Why not importlib: https://docs.python.org/3.6/library/importlib.html?

>>> import importlib            
>>> mod = importlib.import_module('os.path')                         
>>> print(mod)                  
<module 'posixpath' from '/usr/lib64/python3.7/posixpath.py'>
>>> import os.path              
>>> print(os.path)              
<module 'posixpath' from '/usr/lib64/python3.7/posixpath.py'>
BASEARCH = get_basearch()


class ImageError(Exception):

This comment has been minimized.

Copy link
@ashcrow

ashcrow Nov 22, 2019

Member

nit: This may make sense at a higher scope.

This comment has been minimized.

Copy link
@ashcrow

ashcrow Nov 22, 2019

Member

NM this is the higher scope.

_Build.__init__(self, *args, **kwargs)


def __del__(self):

This comment has been minimized.

Copy link
@ashcrow

ashcrow Nov 22, 2019

Member

nit: This may make sense at a higher class.

This comment has been minimized.

Copy link
@ashcrow

ashcrow Nov 22, 2019

Member

Never mind... this is the higher class.

self.meta_write()


class AWSImage(_QemuImage):

This comment has been minimized.

Copy link
@ashcrow

ashcrow Nov 22, 2019

Member

I feel like these should either move out of this module, or the module should be renamed from qemu to something more generic.

Copy link
Member

ashcrow left a comment

Thank you for starting this!!!

@ashcrow ashcrow added the enhancement label Nov 22, 2019
IMG_CLASS_MAP = {
"aliyun": "cosalib.qemu.AliyunImage",
"aws": "cosalib.qemu.AWSImage",
"azure": "cosalib.qemu.AzureImage",

This comment has been minimized.

Copy link
@cgwalters

cgwalters Nov 22, 2019

Member

One recent trend in programming languages has been away from object orientation. While there are a few problem domains it matches really well (GUI programming), I think having good code organization (like Rust modules, Go packages) with basic procedures and interface types is generally better - more flexible, doesn't constrain one as much into thinking of "is-a" relationships.

Specifically for us, I think this code is a good example of when object orientation isn't a great fit - we have a list here of known subclasses.

And in general build software seems very much to me a problem domain that's procedural with functional parts.

That doesn't mean we don't create objects - Python clearly needs them, I just I think it'd be more natural to basically use classes as "structs with methods" - no inheritance. So e.g. the buildextend-aws would call build_image({ platformid: 'aws', 'image_suffix': 'vmdk'}) etc., and that may end up creating an ImageBuild struct with convenient parameters, but there wouldn't be subclassing.

That all said, this is definitely an improvement overall! So none of this is a blocker, just writing out my thoughts, and we should try to be sure we're aligned on future directions.

This comment has been minimized.

Copy link
@cgwalters

cgwalters Nov 22, 2019

Member

(Obviously, Python the language is heavily about objects so it's hard to avoid entirely, and doing so wouldn't be worth it! I just program in Rust and Go (and C) enough that I don't tend to think "subclass" unless the problem domain really really needs it)

This comment has been minimized.

Copy link
@darkmuggle

darkmuggle Nov 22, 2019

Author Contributor

I appreciate the thought and the discussion, and I agree that having an interface over OOO would be nicer. While hacking on this I had similar thoughts, especially around the _Build Class.

Though, the feedback is valuable. I'm seeing a clear case where the VMWare OVA can become a subclass, but the others are weaker cases, especially when it amounts to settings some **kwargs

@cgwalters

This comment has been minimized.

Copy link
Member

cgwalters commented Nov 22, 2019

Are you planning on moving buildextend-metal into Python with this? If so let's be sure no one else is hacking on it at the time as it'll heavily conflict.

@darkmuggle darkmuggle force-pushed the darkmuggle:class-builds branch 2 times, most recently from 57c00e0 to 9e1d5b7 Dec 2, 2019
@darkmuggle darkmuggle force-pushed the darkmuggle:class-builds branch from 9e1d5b7 to e0c84c8 Dec 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.