Implement init tasks #28

zancas · 2016-12-30T05:35:13Z

Fixes #25

Check out this branch.

In an appropriate environment:

sudo pip install --upgrade containenv
cd SOMEPROJECT && containenv init
cd .containenv && bash runcontainenv.sh

zancas · 2016-12-30T05:35:37Z

Fixing flake8 errors now.

dangle · 2016-12-30T05:52:05Z

containenv/containenv/cli/commands/init.py

@@ -11,7 +11,7 @@
 # Unless required by applicable law or agreed to in writing, software
 # distributed under the License is distributed on an "AS IS" BASIS,
 # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-# See the License for the specific language governing permissions and
+# See the License for the specific dependency governing permissions and


Global replace error?

dangle

I think most of this looks pretty good, but I think pulling the config generation out of init into its own thing should be fairly high priority, if I'm understanding that section correctly.

dangle · 2016-12-30T05:56:33Z

containenv/containenv/cli/commands/init.py

 import os
+import logging
+from pkg_resources import resource_filename


Minor style notes: from statements should go above import statements, and pkg_resources isn't part of the standard library, so it should be with jinja2

Huh... did pkg_resources become standard library in Python 3.x?

I don't think it's standard in Python 3.x either, although I'm not entirely sure. It's part of setuptools which just seems ubiquitous.

distutils is part of the standard library, but setuptools is a separate library.

My docker experiments lead me to believe that pkg_resources IS part of the standard library as of Python 2..12.

dangle · 2016-12-30T07:04:01Z

containenv/containenv/cli/commands/init.py

+                'containenv.config.dockerfile_templates',
+                self.from_image), 'r') as fh:
+            self.dockerfile_template = fh.read()
+        logger.warning('self.metasource: {}'.format(self.metasource))


When formatting log messages you should use the old style syntax and pass your format parameters as args to the logger method.

logger.warning('self.metasource: %s', self.metasource)

dangle · 2016-12-30T07:11:37Z

containenv/containenv/cli/commands/init.py

+
+    def _check_project_existence(self):
+        '''verify that self.proj_path is a valid project'''
+        if not os.path.isdir(self.proj_path):


How can this ever be False? Something with metasource? Why not just imply that it's cwd the way git init does?

In case the User uses the tool improperly?

I think I don't really understand why we're allowing the user to specify metasource. Why not just always use the current directory?

If the User passes in an invalid target (not a directory) I want to handle/report it ASAP. Is this OK? @dangle I think we need to discuss this a little more.

dangle · 2016-12-30T07:12:34Z

containenv/containenv/cli/commands/init.py

+                     'argument it received was: {}\nwhich does not map to '
+                     'a path to an existing directory.'
+                     ''.format(self.metasource))
+            raise ProjectDirectoryDoesNotExist(error)


Since you're using a custom exception, I'd put most of this message in the exception class and only pass in self.metasource.

dangle · 2016-12-30T07:13:28Z

containenv/containenv/cli/commands/init.py

+
+    def _make_task(self, method):
+        '''create a run_tasks compliant task'''
+        return (method.__doc__, method)


I think using method.__doc__ is a neat trick, but I'm not sure how i18n will work when doing that.

Is i18n even something we're concerned with?

But not so concerned that I think we should block this PR on it. This is tracked in issue #29.

dangle · 2016-12-30T08:18:26Z

containenv/containenv/cli/commands/test/test_init.py

+    '''The argument does not _already_ possess a ".containenv".'''
+    init_command = Init(testdirectory)
+    init_command.make_path()
+    assert init_command.check_initialization_state() is None


Add an assertion message, por favor.

dangle · 2016-12-30T08:20:29Z

containenv/containenv/cli/commands/test/test_init.py

+    assert init_command.check_initialization_state() is None
+
+
+# tests post self.make_containenv_dir


Make this the docstring?

dangle · 2016-12-30T08:21:09Z

containenv/containenv/cli/commands/test/test_init.py

+def test__check_initialization_state_already_initialized(testdirskeleton):
+    '''The argument _already_ (incorrectly) possesses a ".containenv".'''
+    testdirectory, CONTAINENVPATH, SUBDIRPATH, init_command = testdirskeleton
+    EXPECTED_ERROR = ('init can only be called once per project, but the {}\n'


Same as above. Test the value, not the message.

OK, can we leave the fragiles in, and use best practice going forward.. (e.g. change to value checks WHEN the messages change?)

dangle · 2016-12-30T08:24:24Z

containenv/containenv/cli/commands/init.py

+                self._register_file(dirpath, fname, extension)
+            for dirn in dirs:
+                self._register_dirs(dirpath, dirn, dirs)
+        if not self.registered_dependency_catalog:


Is this really an error? Can I run this on an empty directory?

You shouldn't be running this on an empty directory. Should you?

I've been thinking of this in terms of git init and virtualenv ..

git init can happen at any time, and is often one of the first steps of the project (otherwise, I'm probably cloning).
virtualenv . is pretty much the very first part of creating a new project. You create your virtualenv, activate it, then start working on your code from there. I was thinking containenv would do the same thing. I'd create my project folder, run containenv init in it, contain to activate, then start working inside my container.

I think containenv has a contract with the User that says:

You give me a directory structure with source code, and meta-source code

I'll give you:

an image config (including entrypoint and runner)

an image

I'll run that image and give you:

a contained environment for that source code you gave me.

If the User gives containenv an empty directory containenv breaks saying: You violated our contract. If the User gives containenv a directory with 0 files that containenv recognizes... containenv breaks saying, you violated our contract.

I added this blurb to the README.

I think supporting empty directories is a pretty useful feature. If I want this for development, I want it for all of development, not just the end of it.

I'll log that that as a separate issue though.

dangle · 2016-12-30T08:35:59Z

containenv/containenv/cli/commands/test/test_init.py

+        {fnames[3], SUBDIRPATH, CONTAINENVPATH}
+
+
+BASH_GOT_GIT_MAKE_TMPL = 'FROM        ubuntu:latest\n' +\


Use r"""blah blah blah""" for this so you don't have all the line wrapping and escaping.

OK, can we fix this later... I'm reluctant to mess with it since it works, and flake8 doesn't complain at me.

zancas · 2016-12-30T16:02:17Z

I'll address these comments this afternoon.

If it's OK with you @dangle, I'll probably make some of them (e.g. support other version control systems) into new issues, rather than block this PR.

zancas · 2017-01-02T19:48:29Z

OK, As Far As I Can Tell, AFAICT, this is ready to merge.

zancas · 2017-01-02T22:15:05Z

@dangle what do you think?

Za Wilgustus added 29 commits December 22, 2016 19:09

add method framework for init

bafa4ec

Begin adding tests for init tasks

f56b874

wip implementing and test init tasks

67ed2b4

add pytest_plugins stub

101ec78

pytest_plugins/__init__.py

b37c42d

tests pass, _check_project_existence implemented

21b5643

test that extant projects are handled correctly

2bdc7cc

wip begin implementing dependency detector

def582e

wip

86ef269

wip build dependency catalog

fb1198c

wip create dependency catalog

734dac1

unregistered is a set

fe80cfc

wip maps --> dependencymaps

cfaf7f9

wip check for proper name/ext registration

0eb0c9e

wip, handle directory component properly

d64589f

short circuit on dir depend e.g. .git

c3bf421

Make testdirgeneric fixture

7a34099

Add "self.from_image" to init

22ead37

make apt installs sorted for test convenience

4c0c8d1

Add template

9166eac

begin migrating to pkg_resources approach

6c6ea52

wip, use pkg_resources etc..

ac71308

readd write_entrypoint

8f0bc41

Begin implementing ENTRYPOINT

b31db51

wip entrypoint.sh

fbbc61d

fix flake8 issues

8687a96

add entrypoint.sh

ddcb557

Add tag creation

24d5c4d

First draft complete.

3ec2f49

zancas assigned dangle Dec 30, 2016

zancas requested review from dangle, jputrino and szakeri December 30, 2016 05:35

Za Wilgustus added 2 commits December 30, 2016 05:44

fix flake8 violations

0999fc9

Add blanks after functions, flake8 fix

d63e71d

dangle reviewed Dec 30, 2016

View reviewed changes

Za Wilgustus added 4 commits January 2, 2017 18:01

Address formatting, import style reviews

15a38f8

Migrate custom exception messages into exceptions

c2c0937

Add contract draft, rm useless write

18bc33f

update checks --> asserts

65fa93a

zancas mentioned this pull request Jan 2, 2017

implement containenv init tasks #25

Closed

Za Wilgustus added 2 commits January 2, 2017 15:06

Add User to sudoers etc.

d0af0c6

fixup flake8

4ee645e

dangle assigned zancas and unassigned dangle Jan 2, 2017

dangle merged commit 50ff009 into development Jan 2, 2017

dangle deleted the implement_init_tasks branch January 15, 2017 05:51

		assert init_command.check_initialization_state() is None


		# tests post self.make_containenv_dir

		{fnames[3], SUBDIRPATH, CONTAINENVPATH}


		BASH_GOT_GIT_MAKE_TMPL = 'FROM ubuntu:latest\n' +\

Implement init tasks #28

Implement init tasks #28

Conversation

zancas commented Dec 30, 2016

zancas commented Dec 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dangle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zancas commented Dec 30, 2016 • edited Loading

zancas commented Jan 2, 2017

zancas commented Jan 2, 2017

zancas commented Dec 30, 2016 •

edited

Loading