Skip to content
This repository has been archived by the owner. It is now read-only.

Refactor nulecule base [Fixes #145] #278

Merged
merged 35 commits into from Nov 3, 2015

Conversation

@rtnpro
Copy link
Member

commented Sep 18, 2015

This includes initial work on refactoring Nulecule base code, as discussed in #145 .


class Nulecule(NuleculeBase):
"""
This represents a Nulecule application. A Nulecule instance can have

This comment has been minimized.

Copy link
@goern

goern Sep 18, 2015

Contributor

just this morning a little picky... "an Application compliant with the Nulecule Specification" ;)


class NuleculeComponent(NuleculeBase):
"""
Represents a local service in a Nulecule application. It receives props

This comment has been minimized.

Copy link
@goern

goern Sep 18, 2015

Contributor

What do you mean by 'service'?

This comment has been minimized.

Copy link
@rtnpro

rtnpro Sep 18, 2015

Author Member

service == graph item, or dependency, etc

This comment has been minimized.

Copy link
@goern

goern Sep 18, 2015

Contributor

why dont we call 'a component' rather than 'local service' ?

This comment has been minimized.

Copy link
@rtnpro

rtnpro Sep 18, 2015

Author Member

Agreed 👍

@goern goern added this to the CDK 2 beta-3 milestone Sep 18, 2015

@vpavlin

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2015

Hmm..you are quick, guys:) I put this together yesterday: https://gist.github.com/vpavlin/f46444efd647e0d8a48f

Maybe I missed some discussion, so if that's the case, can we reiterate on it? Or if that didn't happen, can I have one, please?:)

self.requirements = requirements
self.params = params or []
self.namespace = namespace
self.config = self.load_config(config, self.params, self.namespace)

This comment has been minimized.

Copy link
@vpavlin

vpavlin Sep 21, 2015

Contributor

What about moving this to load(), or moving load_components() out of load ?

@aweiteka

This comment has been minimized.

Copy link
Contributor

commented Sep 22, 2015

I haven't done a full code review but I agree this is in the right direction. It will help with my work in progress #142 .

@vpavlin do you consider this foundational to address your gist?

@vpavlin

This comment has been minimized.

Copy link
Contributor

commented Sep 23, 2015

@aweiteka Yes, I think this is going into the right direction:)

@landscape-bot

This comment has been minimized.

Copy link

commented Sep 23, 2015

Code Health
Repository health decreased by 1% when pulling 591ac68 on rtnpro:refactor_nulecule_base into cb931ec on projectatomic:master.

@cdrage

This comment has been minimized.

Copy link
Member

commented Sep 23, 2015

👍 on the docker-py, if we decide to use that we can interact with the docker commands more programatically.

although this does add another dependancy...

@goern thoughts?

@landscape-bot

This comment has been minimized.

Copy link

commented Sep 27, 2015

Code Health
Repository health decreased by 2% when pulling ef755c0 on rtnpro:refactor_nulecule_base into cb931ec on projectatomic:master.

@landscape-bot

This comment has been minimized.

Copy link

commented Oct 6, 2015

Code Health
Repository health decreased by 5% when pulling fa2aa73 on rtnpro:refactor_nulecule_base into 80d17bf on projectatomic:master.

@aweiteka

This comment has been minimized.

Copy link
Contributor

commented Oct 7, 2015

@rtnpro nice work! LGTM

@cdrage

This comment has been minimized.

Copy link
Member

commented Oct 7, 2015

@rtnpro

Few questions,

  • Is there any possibility to add some testing?
  • Is it bad naming practice to have both a NuleculeBase and Nulecule_Base? As that's what we currently have
self.app_name = '{}-{}'.format(
Utils.sanitizeName(self.image), uuid.uuid1())
self.nulecule = None
self.unpack_path = os.path.join(CACHE_DIR, self.app_name)

This comment has been minimized.

Copy link
@cdrage

cdrage Oct 7, 2015

Member

What is /var/atomicapp doesn't exist? fails?

@cdrage

This comment has been minimized.

Copy link
Member

commented Oct 7, 2015

@rtnpro can you rebase too so I can git clone your branch and test it :)

@rtnpro rtnpro force-pushed the rtnpro:refactor_nulecule_base branch from fa2aa73 to 783940c Oct 7, 2015

@rtnpro

This comment has been minimized.

Copy link
Member Author

commented Oct 7, 2015

@charliedrage I just rebased the branch :)

@rtnpro

This comment has been minimized.

Copy link
Member Author

commented Oct 7, 2015

On Wed, Oct 7, 2015 at 6:20 PM, Charlie Drage notifications@github.com wrote:

@rtnpro

Few questions,

Is there any possibility to add some testing?

Yes, of course, and I will start to work on it, to wire up this new
package and cover all edge cases.

Is it bad naming practice to have both a NuleculeBase and Nulecule_Base? As that's what we currently have

It is, indeed. Python suggests use of CapWords for class names and
_ (underscore) separated names for variable and function names for
better readability. You can read more about it here:
https://www.python.org/dev/peps/pep-0008/#function-names. However, in
our current code, we have a mix of various styles. I'm more inclined
to stick to the PEP8 guidelines, and it reflects in the newer code of
nulecule package. In time, we'll refactor the entire code base to
follow a consistent naming convention. Does it make sense?

@rtnpro

This comment has been minimized.

Copy link
Member Author

commented Oct 7, 2015

@charliedrage ^^

@rtnpro rtnpro force-pushed the rtnpro:refactor_nulecule_base branch from 783940c to dd0e7be Oct 8, 2015

@landscape-bot

This comment has been minimized.

Copy link

commented Oct 9, 2015

Code Health
Repository health decreased by 4% when pulling dd0e7be on rtnpro:refactor_nulecule_base into 5d81806 on projectatomic:master.

@LalatenduMohanty

This comment has been minimized.

Copy link
Member

commented Oct 9, 2015

@rtnpro Code is more readable now :)

@@ -29,6 +29,7 @@
EXTERNAL_APP_DIR = "external"
GLOBAL_CONF = "general"
APP_ENT_PATH = "application-entity"
CACHE_DIR = "/var/atomicapp"

This comment has been minimized.

Copy link
@vpavlin

vpavlin Oct 9, 2015

Contributor

Make it /var/lib/atomicapp, please:)

This comment has been minimized.

Copy link
@rtnpro

rtnpro Oct 13, 2015

Author Member

Done!

words = line.split()
image_name = words[0]
registry = repo = None
if image_name.find('/') >= 0:

This comment has been minimized.

Copy link
@vpavlin

vpavlin Oct 9, 2015

Contributor

Maybe use `if "/" in image_name"?

This comment has been minimized.

Copy link
@rtnpro

rtnpro Oct 9, 2015

Author Member

👍

This comment has been minimized.

Copy link
@rtnpro

rtnpro Oct 13, 2015

Author Member

Done!

image_name = words[0]
registry = repo = None
if image_name.find('/') >= 0:
registry, repo = image_name.split('/', 1)

This comment has been minimized.

Copy link
@dustymabe

dustymabe Oct 9, 2015

Contributor

what happens here if there is no registry for an image? For example if the image was built on the local machine.

This comment has been minimized.

Copy link
@vpavlin

vpavlin Oct 9, 2015

Contributor

see next line - image_name == image

for group, group_vars in from_config.items():
to_config[group] = to_config.get(group) or {}
if group == GLOBAL_CONF:
if self.namespace == GLOBAL_CONF:

This comment has been minimized.

Copy link
@vpavlin

vpavlin Oct 9, 2015

Contributor

This if is a dead code, isn't it?

If self.namespace is GLOBAL_CONF, use GLOBAL_CONF (== self.namespace), otherwise, use self.namespace...i.e. it's self.namespace everytime..

def __init__(self, id, specversion, metadata, graph, basepath,
requirements=None, params=None, config=None,
namespace=GLOBAL_CONF):
self.id = id

This comment has been minimized.

Copy link
@vpavlin

vpavlin Oct 9, 2015

Contributor

These should be declared in NuleculeBase as methods there are using them and it's not cleare where they come from..(and then obviously defined here)

Let me restate..some of these need to be declared in NuleculeBase - for example namespace is used in the NuleculeBase method load_config but it's not clear where did it come from..

This comment has been minimized.

Copy link
@rtnpro

rtnpro Oct 13, 2015

Author Member

Makes sense 👍

This comment has been minimized.

Copy link
@rtnpro

rtnpro Oct 13, 2015

Author Member

Done!

rtnpro added some commits Oct 11, 2015

Move arguments for answers to "run" command alone. #310
Running a Nulecule application now generates a file
named "answers.conf.gen" in the application directory, which
contains all config data (answers + user input) used to run
the application. The same data is loaded and used to stop
the application without asking the user for any input.
Miscellaneous fixes based on code review
- use 'in' to find a char in string rather than str.find
- strip of 'docker://' prefix in source image name using Utils.getSourceImage
- Update CACHE_DIR to '/var/lib/atomicapp'
Streamlined install flow
- added docs
- added logs
- fixed dry-run, nodeps argument handling
Fix run flow
- Fix generating paths for rendered artifacts, to feed to provider
- Prevent deploying external application multipled times.
Don't overload Nulecule.load_from_path to copy directory to target path.
If at all it is needed to copy a Nulecule application directory
to another path, when --destination option is provided in
atomicapp install command, then the caller will copy the application
directory to the destination first, and then call Nulecule.load_from_path
to load the Nulecule application from the destination path.
Enable skip_asking option during loading config
and fix 'ask' option for load_config as well. This commit allows us to load_config
parameters for the entire nulecule application during install and dumping it to
answers.conf.sample without prompting the user for any input.

@rtnpro rtnpro force-pushed the rtnpro:refactor_nulecule_base branch from 2501bad to 54bf04c Oct 15, 2015

@rtnpro

This comment has been minimized.

Copy link
Member Author

commented Oct 15, 2015

Rebased!

@dustymabe

This comment has been minimized.

Copy link
Contributor

commented Oct 15, 2015

Ok this has been incorporated in https://github.com/projectatomic/atomicapp/commits/cdk2-beta3-rc1 I'm going to lock comments here.. new things can go into issues/PRs to https://github.com/projectatomic/atomicapp/commits/cdk2-beta3-rc1.

@projectatomic projectatomic locked and limited conversation to collaborators Oct 15, 2015

@goern goern added the blocked label Oct 15, 2015

@cdrage cdrage removed the blocked label Oct 29, 2015

@dustymabe dustymabe merged commit 54bf04c into projectatomic:master Nov 3, 2015

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/travis-ci/push The Travis CI build failed
Details
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
8 participants
You can’t perform that action at this time.