Merge has line #113

jbzdak · 2013-01-30T21:18:51Z

Here is proposed implementation of ensure_has_line that is coircumvents shell escapes problem, but has own limitations (that is: it disallows """ in line).

It relies on execute_python_script from latter commit, but I posted it as separate pull request.

It is tested in the sense that I checked that it works, but I just fixed provy tests that failed because of register_loader in core.Role, and wrote no new tests.

It works like that:

Creates a python script that checks for line and append it if necessary
Uploads the script to remote server and executes it.

What do you think about it?

(cherry picked from commit b75d6bd)

diogobaeder · 2013-01-31T01:39:05Z

We have lots of fixes and improvements on #112, so I'd say let's wait for that other one to be ready and merged, before we go on with this one. I'm not sure how your newer commits in this pull request will play with the other ones after you fix the things I asked you to.

jbzdak · 2013-01-31T15:48:46Z

There are big regressions in this, because of

       self.register_template_loader("provy.core")

in Role.__init__ many tests fail, because register_template_loader dies when called on role with empty context.

Merge remote-tracking branch 'remotes/upstream/master' into merge_has_line Conflicts: tests/unit/core/test_roles.py

diogobaeder · 2013-01-31T17:43:47Z

OK, time to review this pull request now that the previous one merged.

I see some problems with it, which I'd like you to fix and improve:

You've put the script template inside the role method. This is really ugly, not only it's a big string which should be in a separate file, but it also breaks the visual organization of the code because of the indentation inside the string. As I've told you about that big string for public key, please keep such big strings of data in separate files. They just pollute our code;
Instead of not allowing the user to put """, why don't you just write the line to a separate temp file, and read this separate temp file inside that script? This would allow us to put anything we want, as a line, inside the target file;
Please test that script. If you do the changes mentioned above, there will be no reason to stop you from testing it.

jbzdak · 2013-01-31T17:48:54Z

As for placing script in additional temp file I didn't think of it, and it is a great idea.

As for template inside method I did that at first, but then I had to register_template_loader in __init__, which breaked almost all tests, because register_template_loader dies when context is empty.

I have no ide what to do with these errors:

Should I somehow fix the register_template_loader.
Should I fix context in all the tests?
Anything else?

diogobaeder · 2013-02-01T01:22:28Z

I think I may be able to help you with this. Take a look here:
https://github.com/python-provy/provy/blob/master/provy/core/runner.py#L49

We have a ChoiceLoader, which already has a FileSystemLoader, being setup when provy is run from the command line. So what you need to do is have this loader registered as part of the test, and have that script inside "provy/files/". Just point the loader from your test to "provy/files/" as well, and it will work fine.

Let me know if this helps you.

jbzdak · 2013-02-01T09:07:11Z

It makes sense.

I'll see to it sometime during next couple of days.
2013/2/1 Diogo Baeder notifications@github.com

We have a ChoiceLoader, which already has a FileSystemLoader, being setup
when provy is run from the command line. So what you need to do is have
this loader registered as part of the test, and have that script inside
"provy/files/". Just point the loader from your test to "provy/files/" as
well, and it will work fine.

jb:)

jbzdak · 2013-02-06T21:45:00Z

provy/core/roles.py

@@ -67,9 +67,15 @@ def __init__(self, prov, context):
            context['used_roles'] = {}
        if 'roles_in_context' not in context:
            context['roles_in_context'] = {}
+        if not "registered_loaders" in context:


I gathered that it would be better to just add needed keys to context, in this case Role wold be ready to work even if started with empty context.

I don't really get why you're changing these context items here. There's no need to do so, as provy/core/runner.py already does that when it's run from the command line.

If you need to change the context for tests, then change it for tests only, by providing an appropriate context to the role instance in the tests, instead of letting it leak to the production code.

Bacisally without this lines you coiuldn't write:

role = Role(whatever, {})

because you'll end up with KeyError raised from register_template_package. It would totally break Object Oriented Padagrim.

The thing is, the main loader directory is defined at runtime. This is guaranteed by the application, and even has automated test coverage.

So you can just assume that at runtime it will provide a filesystem loader, with the correct directory, and provide one yourself on your tests. I don't see how it violates any OOP principle. If you want to see this from an OOP point of view, just think of Inversion of Control. ;-)

jbzdak · 2013-02-06T21:48:34Z

There is one thing that needs to be discussed: what does owner switch as I look into prior implementation it seems that owner was noop unless we created this file.

diogobaeder · 2013-02-06T23:21:57Z

provy/core/templates/__init__.py

@@ -0,0 +1 @@
+__author__ = 'jb'


Please remove.

jbzdak · 2013-02-10T19:04:19Z

Superseeded by next merge that is way better.

First version of ensure_line

00d53eb

(cherry picked from commit b75d6bd)

New version that is somewhat safer

f1161db

Merge remote-tracking branch 'remotes/upstream/master' into merge_has_line Conflicts: tests/unit/core/test_roles.py

jbzdak added 3 commits February 6, 2013 21:08

Merge remote-tracking branch 'upstream/master' into merge_has_line

9ab851e

new has_line_changes

a2498a7

Removed unnecessary check

594065a

jbzdak reviewed Feb 6, 2013
View reviewed changes

Deleted temporary files created during tests.

4535648

diogobaeder reviewed Feb 6, 2013
View reviewed changes

provy/core/templates/__init__.py

@@ -0,0 +1 @@

__author__ = 'jb'

Copy link

Contributor

diogobaeder Feb 6, 2013

Choose a reason for hiding this comment

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

Please remove.

jbzdak closed this Feb 10, 2013

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Merge has line #113

Merge has line #113

Uh oh!

jbzdak commented Jan 30, 2013

Uh oh!

diogobaeder commented Jan 31, 2013

Uh oh!

jbzdak commented Jan 31, 2013

Uh oh!

diogobaeder commented Jan 31, 2013

Uh oh!

jbzdak commented Jan 31, 2013

Uh oh!

diogobaeder commented Feb 1, 2013

Uh oh!

jbzdak commented Feb 1, 2013

Uh oh!

jbzdak Feb 6, 2013

Uh oh!

diogobaeder Feb 6, 2013

Uh oh!

jbzdak Feb 7, 2013

Uh oh!

diogobaeder Feb 7, 2013

Uh oh!

jbzdak commented Feb 6, 2013

Uh oh!

diogobaeder Feb 6, 2013

Uh oh!

jbzdak commented Feb 10, 2013

Uh oh!

Uh oh!

Merge has line #113

Merge has line #113

Uh oh!

Conversation

jbzdak commented Jan 30, 2013

Uh oh!

diogobaeder commented Jan 31, 2013

Uh oh!

jbzdak commented Jan 31, 2013

Uh oh!

diogobaeder commented Jan 31, 2013

Uh oh!

jbzdak commented Jan 31, 2013

Uh oh!

diogobaeder commented Feb 1, 2013

Uh oh!

jbzdak commented Feb 1, 2013

Uh oh!

jbzdak Feb 6, 2013

Choose a reason for hiding this comment

Uh oh!

diogobaeder Feb 6, 2013

Choose a reason for hiding this comment

Uh oh!

jbzdak Feb 7, 2013

Choose a reason for hiding this comment

Uh oh!

diogobaeder Feb 7, 2013

Choose a reason for hiding this comment

Uh oh!

jbzdak commented Feb 6, 2013

Uh oh!

diogobaeder Feb 6, 2013

Choose a reason for hiding this comment

Uh oh!

jbzdak commented Feb 10, 2013

Uh oh!

Uh oh!