Looks OK to me. +1

Is requests not pulled in by requests_oauthlib?

I just checked and it's pulled in, so I think you can just add this to the requirements.txt.

Is there a reason this doesn't use flask-oidc?

Is requests not pulled in by requests_oauthlib?
Yeah, not sure why tox complained that requests was missing last time.

Is there a reason this doesn't use flask-oidc?

It's because flask-oidc is not in EPEL7.

Well, we could either get flask-oidc into EPEL7 :-) or drop EPEL7 as a deployment target, and go for Fedora 25. I don't think anyone ever made any decision about that -- I was just always trying to target EPEL7 by default, because that's what Fedora infra prefers.

Just depends whether using flask-oidc is a lot nicer than not using it?

Well, we could either get flask-oidc into EPEL7 :-) or drop EPEL7 as a deployment target, and go for Fedora 25. I don't think anyone ever made any decision about that -- I was just always trying to target EPEL7 by default, because that's what Fedora infra prefers.
Just depends whether using flask-oidc is a lot nicer than not using it?

I think it would be nice to use it, then we have less code to maintain.

Would be nice to just refer to some other existing copy of this script (there must be many), rather than keeping our own copy committed here -- I am assuming there is nothing waiverdb specific in it.

Your first commit is making flake8 happy, not tox (maybe just tweak the commit message).

BTW if we are planning to keep flake8 happy we better get it into the Jenkinsfile otherwise we will keep accidentally regressing on it I'm sure.

Okay, I will take it out and probably move it to my own repo.

I would like to get this merged first as it is working. Then I will try out flask-oidc and see if it would make things nicer or not.

Ah ha.. python-flask-oidc has an epel7 branch: https://admin.fedoraproject.org/pkgdb/package/rpms/python-flask-oidc/

But there are no builds or updates for it: https://apps.fedoraproject.org/packages/python-flask-oidc/

The python-flask-oidc package is well-done and looks nearly ready for EPEL7 with py3 conditionals.

Here's a scratch build which shows that we're missing another dep: https://koji.fedoraproject.org/koji/taskinfo?taskID=18664993

That is python-oauth2client which also has an epel7 branch, but no build or update.

That missing build is tracked here. https://bugzilla.redhat.com/show_bug.cgi?id=1356121

I've applied for co-maintainership of both.

@mbaldessari and @skrzepto - do either of you mind if I help with EPEL7 branches for python-flask-oidc and python-oauth2client?

I've applied for rights:

• https://admin.fedoraproject.org/pkgdb/package/rpms/python-oauth2client/
• https://admin.fedoraproject.org/pkgdb/package/rpms/python-flask-oidc/

Trail of blockers: https://bugzilla.redhat.com/showdependencytree.cgi?id=1437531

Okay :-) so in that case, it seems like we should go ahead with this approach (not using flask-oidc) and we can revisit later in case flask-oidc makes things simpler for us.

Is this file supposed to be committed? Whatever this secret is, it's not a secret anymore :-)

Is this more like, an example config for development purposes? In that case, it makes sense I guess... Will it work unmodified for anyone who wants to run it? And does it matter that we have now leaked this secret here?

Seems like an unrelated change? What's the reason for it?

flask-oidc was built for EL7 as of yesterday.

Yes, it is used for development purpose. It should work for anyone who has a FAS account. There is nothing to be worried about as the secret is generated from the development server.

Yeah, I should move it into a separate patch as I was convinced by this patch in the mbs

https://pagure.io/fm-orchestrator/c/a4763ee316e23d89eabbfa55ad4f3655128bcbb0

flask-oidc was built for EL7 as of yesterday.

Cool, I'll give it go with flask-oidc.

LGTM, :+1:

@dcallagh any feedback as I want to get this one merged?

Sorry I didn't realise this was ready for review again. Pagure does not send any notifications when you amend the PR. @mjia in future we all better remember we have to write a comment in that case.

:+1: nice work!

Sorry I didn't realise this was ready for review again. Pagure does not send any notifications when you amend the PR. @mjia in future we all better remember we have to write a comment in that case.

I will add your email address in the public notifications and you should be able to receive a notification when a new comment is made.

No I already get comments on the PRs.

The problem is 4 days ago you say "I'll try using flask-oidc", then 2 days ago you posted the amended version which uses it but you didn't write any comment at that point. So I didn't realise you had actually posted it.

Okay, I see what you mean. Pagure doesn't send any notification when the patch is reabased, sigh!