@dpatrick and @Bawolff Can I get a guesstimate on when you might get to this review? The core code is complete for the initial needs and I'm trying to decide if I should tidy things up and start planning a deployment plan or continue on to add more features prior to review and deployment.

@bd808 How does the week of July 4th and into part of the week of the 11th work for you? That's the earliest we can schedule this.

In T135784#2362070, @dpatrick wrote:

@bd808 How does the week of July 4th and into part of the week of the 11th work for you? That's the earliest we can schedule this.

Sounds great! I'll start planning for other follow on events accordingly.

@dpatrick any status update on this review?

Overall, really nice job on entire application. This is a great example of solid Django development. I'm really into striker/settings.py and the use of an ini file. The app's handling of trusted proxies, CSP, CSRF protection by default, rate-limiting connections to LDAP--all great.

Non-security issues:

• Vagrant tools cannot find Python.h; needs to depends on -dev package
• Database calls fails if roles is enabled prior to first "vagrant up"

Non-security questions:

• Are tools in Tool Labs now required to only use Diffusion? In other words, do tool repos already hosted elsewhere (Github) have to be imported to Diffusion?

striker/tools/models.py, line 78

• fix TODO for field lengths

striker/labsauth/views.py, line 48

• Should session expiry be an ini-based config parameter?

In T135784#2472547, @dpatrick wrote:

Overall, really nice job on entire application. This is a great example of solid Django development. I'm really into striker/settings.py and the use of an ini file. The app's handling of trusted proxies, CSP, CSRF protection by default, rate-limiting connections to LDAP--all great.

Thanks. :)

I tried several different config management ideas and the ini-file one seemed like it would be easiest to manage via Puppet, plus I like the added bonus of non-executable config. That's one aspect of Django that has always bugged me a little.

Non-security issues:

• Vagrant tools cannot find Python.h; needs to depends on -dev package
• Database calls fails if roles is enabled prior to first "vagrant up"

Good catches. I'll touch up the mw-vagrant role and try to get these fixed.

Non-security questions:

• Are tools in Tool Labs now required to only use Diffusion? In other words, do tool repos already hosted elsewhere (Github) have to be imported to Diffusion?

No. The intent here is just to make it really simple to setup version control for a tool. Using some other VCS (github, bitbucket, whatever) is fine.

I would like to add support for mirroring externally hosted git VCS into Diffusion to make discovery of source code easier for folks who want to help fix up an existing tool. It would also be nice to add mirroring out to github for folks who like to keep track of their contributions there. Neither of these seemed necessary for the initial release functionality however.

striker/tools/models.py, line 78

• fix TODO for field lengths

Thanks for reminding me. I punted on that one during dev but should be able to at least make a lower limit for the phid I think.

striker/labsauth/views.py, line 48

• Should session expiry be an ini-based config parameter?

That sounds like a good idea. That will make tuning things easier if we decide 2 week sessions are too long/short.

In T135784#2472547, @dpatrick wrote:

• Vagrant tools cannot find Python.h; needs to depends on -dev package
• Database calls fails if roles is enabled prior to first "vagrant up"

https://gerrit.wikimedia.org/r/#/c/299708/

In T135784#2472547, @dpatrick wrote:

striker/tools/models.py, line 78

• fix TODO for field lengths

https://github.com/bd808/striker/commit/00771e8fd659b3a8d9ff2dd7f5cad597c1e1ac89

In T135784#2472547, @dpatrick wrote:

striker/labsauth/views.py, line 48

• Should session expiry be an ini-based config parameter?

https://github.com/bd808/striker/commit/ba766d19919ef4edaeb9dd7da4d33580cea43d2a

@dpatrick Can we call this closed now, or are there other issues that you would like to see addressed?

In T135784#2487952, @bd808 wrote:

@dpatrick Can we call this closed now, or are there other issues that you would like to see addressed?

ping?