@ralph @dcallagh I'm not sure about the conf (where I put the check of it...) and the url of the "default" dist-git.

Since the lowest Python version is 2.7 [1] I think you should be able to use "{dist_git_base_url}/{package_name}/{revision}/greenwave.yaml" and remove the comment.

[1] Or it seems to be the case, looking at "tox.ini".

Were these lines accidentally left here?

Look like for me after running the tests, the server process stays up.

Can you use requests-mock instead?

    with requests_mock.mock() as m:
        m.get('http://localhost:5678/.../greenwave.yaml', text='{"data": {}}')
        yield 'http://localhost:5678'

Mmm, no. This is the conf for the test.

Oh, OK, I didn't see the code above diff. :)

Well, there's more than one fake server in here. This distgit_server is just the newest addition. If we change the method for handling their termination, we should do it across the board.

(Either way, I vote to put that kind of cleanup in a separate PR. Can probably get away without requests-mock with a try/finally around the yield.)

Good idea. A template with named variables is much nicer to maintain for admins.

Looks like this got sucked in somehow in a rebase. Can you try a fresh rebase on master to see if they get cleaned out?

One observation. With this config:

# Copy this file to `conf/settings.py` to put it into effect. It overrides the values defined
# in `greenwave/config.py`.
SECRET_KEY = 'lolreplace-me-with-something-random'
HOST = '0.0.0.0'
PORT = 5005
RESULTSDB_API_URL = 'https://taskotron.fedoraproject.org/resultsdb_api/api/v2.0'
WAIVERDB_API_URL = 'https://waiverdb-web-waiverdb.app.os.fedoraproject.org/api/v1.0'
DIST_GIT_BASE_URL = 'https://src.fedoraproject.org'

I was able to get the server to traceback like this:

2018-04-19 19:56:23 [pid   743] werkzeug INFO 127.0.0.1 - - [19/Apr/2018 19:56:23] "POST /api/v1.0/decision HTTP/1.1" 500 -
Traceback (most recent call last):
  File "/home/threebean/.virtualenvs/greenwave2/lib/python2.7/site-packages/Flask-0.12.2-py2.7.egg/flask/app.py", line 1997, in __call__
    return self.wsgi_app(environ, start_response)
  File "/home/threebean/.virtualenvs/greenwave2/lib/python2.7/site-packages/Flask-0.12.2-py2.7.egg/flask/app.py", line 1985, in wsgi_app
    response = self.handle_exception(e)
  File "/home/threebean/.virtualenvs/greenwave2/lib/python2.7/site-packages/Flask-0.12.2-py2.7.egg/flask/app.py", line 1540, in handle_exception
    reraise(exc_type, exc_value, tb)
  File "/home/threebean/.virtualenvs/greenwave2/lib/python2.7/site-packages/Flask-0.12.2-py2.7.egg/flask/app.py", line 1982, in wsgi_app
    response = self.full_dispatch_request()
  File "/home/threebean/.virtualenvs/greenwave2/lib/python2.7/site-packages/Flask-0.12.2-py2.7.egg/flask/app.py", line 1614, in full_dispatch_request
    rv = self.handle_user_exception(e)
  File "/home/threebean/.virtualenvs/greenwave2/lib/python2.7/site-packages/Flask-0.12.2-py2.7.egg/flask/app.py", line 1517, in handle_user_exception
    reraise(exc_type, exc_value, tb)
  File "/home/threebean/.virtualenvs/greenwave2/lib/python2.7/site-packages/Flask-0.12.2-py2.7.egg/flask/app.py", line 1612, in full_dispatch_request
    rv = self.dispatch_request()
  File "/home/threebean/.virtualenvs/greenwave2/lib/python2.7/site-packages/Flask-0.12.2-py2.7.egg/flask/app.py", line 1598, in dispatch_request
    return self.view_functions[rule.endpoint](**req.view_args)
  File "/home/threebean/devel/greenwave/greenwave/utils.py", line 54, in wrapped
    return func(*args, **kwargs)
  File "/home/threebean/devel/greenwave/greenwave/api_v1.py", line 264, in make_decision
    'policies_satisfied': all(answer.is_satisfied for answer in answers),
  File "/home/threebean/devel/greenwave/greenwave/api_v1.py", line 264, in <genexpr>
    'policies_satisfied': all(answer.is_satisfied for answer in answers),
AttributeError: 'InternalServerError' object has no attribute 'is_satisfied'

And... now I see why. :) There's an exception that gets returned instead of raised.

diff --git a/greenwave/policies.py b/greenwave/policies.py
index aafa590..1b0564d 100644
--- a/greenwave/policies.py
+++ b/greenwave/policies.py
@@ -193,7 +193,7 @@ class RemoteOriginalSpecNvrRule(Rule):
         return answers
     # if we arrived here it means that we don't have any result...
-        return InternalServerError('Impossible to extend the policy.')
+        raise InternalServerError('Impossible to extend the policy.')
     def to_json(self):
     return {

Ah, I think I found a problem with the way we get 'rev' from the list of results here. I sent an email writeup about it your way @gnaponie.

I changed what you noticed and now when the "rev" is not found in the result, greenwave is asking to koji. The test for koji is mocked.

Better to use a dedicate tmp directory for this. The tmpdir_factory fixture can help.

https://docs.pytest.org/en/latest/tmpdir.html

And instead of pushd with shell=True you can just use:

Popen([sys.executable, '-m', 'SimpleHTTPServer', '5678'], cwd=tmpdir)

Instead of pulling in koji as a dependency, we can just use a plain xmlrpclib.ServerProxy(). Since there is no authentication needed, we don't need anything special from koji.ClientSession().

Might be nicer to parse this as a url and extract the anchor from there.

url = urlparse(build['extra']['source']['original_url'])
rev = url.fragment

Might be worth checking that the scheme is 'git' also.

Do we definitely want to fetch the greenwave.yml from the exact git revision that was used to build the package? Rather than just, the latest commit on the same branch? Imagine this scenario:

• I have a greenwave.yml requiring mypackage.functionaltest to be passing
• I build the package
• the test fails, and I realise it's no longer valid so I want to change my package's policy to not require it
• I commit a change to greenwave.yml on the same branch
• ... but Greenwave will keep using the contents of the old commit

I guess in this situation, our answer is you have to change your greenwave.yml as desired but also submit a waiver if you want Greenwave to allow that particular build?

Also, does it make sense to have this be a rule in the policy? Obviously this rule makes no sense if you try to use it in your package-specific policy that is already loaded from dist-git...

Could we just always try loading the greenwave.yml, if the suitable config items are in place? When would we ever want to ignore a greenwave.yml that was present in dist-git?

this is here by mistake. I'll remove it.

Changed the PR following Dan's advice.

@ralph what do you think about what Dan said? We were saying in our today's meeting that it could be some consideration for the future, but maybe we should think about it...

Dan's advice is not unreasonable. My counter is that the system gains integrity if we can say that the policy that applies to a specific rev is immutable. Neither approach (latest-on-branch or specific-rev-only) is a hard requirement from any customer. Either works for me. If the koji work is implemented and satisfactorily functional, I'd stick with it and then revise in a future PR.

@dcallagh, if you have time today can you give a fresh round of review on this one? It is end of day for me already. :)

There is one problem with the dist-git process for the functional test. It remains "alive" after the test.
I'll have a look.

It should be everything fine now.
@lholecek can you please check if the process it is still running for you after the funcional-tests? For me now it is fine.

Thanks

+1