Launchpad itself

Code review comment for lp:~jtv/launchpad/bug-526462

bug-526462
Merge into devel

Revision history for this message

Michael Nelson (michael.nelson) wrote on 2010-02-25:

As discussed, we have other damaging scripts intended for local development that do not even do any checks. I do think it would be a simpler (and sufficient) implementation to just check LPCONFIG == 'development', but I'll leave that up to you.

16:42 < jtv> noodles775: thanks for the review!
16:43 < jtv> noodles775: I'm a bit worried though that being this strict would stop people from playing with customized configs.
16:43 < jtv> I realize it's always a tradeoff, but there's also the "do we have a really large production-like table" test.
16:44 < noodles775> jtv: well, if they're playing with customized configs, they're not going to have a problem spending 10 seconds to adjust the script if
                    they want to.
16:45 < jtv> noodles775: true, but this script already has another check to ensure it's not running on production; how many scripts do we have (without ever
             any trouble) that are just as dangerous without anyone ever adding checks like this?
16:45 < jtv> I mean, if I hadn't added the check, would the average reviewer have thought of adding it?
16:45 < noodles775> jtv: which scripts? make schema?
16:46 < jtv> noodles775: I guess, though I've no idea whether that has any checks.
16:46 < noodles775> jtv: yeah, I'm not sure. All the other scripts that I can think of *add* info, not delete.
16:46 < jtv> launchpad-database-setup
16:46 * noodles775 looks
16:47 < jtv> mock-code-import ("warning! run make schema first!")
16:48 < jtv> We also have a script now, apparently, to remove private data. There may be more.
16:49 < noodles775> Yep, you're right.
16:50 < jtv> So I don't want to spend my time guarding against the admin who accidentally goes through the rigmarole for running scripts against a
             production db, with the --force option added.
16:51 < noodles775> yeah, I agree. I was more worried about the situation where a person runs it against a smaller DB with a different config.
16:53 < jtv> noodles775: there's a good chance that the script might fail, and I'd estimate the risk of them running "make schema" by accident considerably
             larger.
16:53 < jtv> (from shell history, f'rinstance)
16:54 < noodles775> jtv: I just thought it would have been a simpler implementation, not more difficult (ie. LPCONFIG == 'development'), but yep, I'm
                    updating the MP now.

16:42 < jtv> noodles775: thanks for the review!
16:43 < jtv> noodles775: I'm a bit worried though that being this strict would stop people from playing with customized configs.
16:43 < jtv> I realize it's always a tradeoff, but there's also the "do we have a really large production-like table" test.
16:44 < noodles775> jtv: well, if they're playing with customized configs, they're not going to have a problem spending 10 seconds to adjust the script if 
                    they want to.
16:45 < jtv> noodles775: true, but this script already has another check to ensure it's not running on production; how many scripts do we have (without ever 
             any trouble) that are just as dangerous without anyone ever adding checks like this?
16:45 < jtv> I mean, if I hadn't added the check, would the average reviewer have thought of adding it?
16:45 < noodles775> jtv: which scripts? make schema?
16:46 < jtv> noodles775: I guess, though I've no idea whether that has any checks.
16:46 < noodles775> jtv: yeah, I'm not sure. All the other scripts that I can think of *add* info, not delete.
16:46 < jtv> launchpad-database-setup
16:46  * noodles775 looks
16:47 < jtv> mock-code-import ("warning! run make schema first!")
16:48 < jtv> We also have a script now, apparently, to remove private data.  There may be more.
16:49 < noodles775> Yep, you're right.
16:50 < jtv> So I don't want to spend my time guarding against the admin who accidentally goes through the rigmarole for running scripts against a 
             production db, with the --force option added.
16:51 < noodles775> yeah, I agree. I was more worried about the situation where a person runs it against a smaller DB with a different config.
16:53 < jtv> noodles775: there's a good chance that the script might fail, and I'd estimate the risk of them running "make schema" by accident considerably 
             larger.
16:53 < jtv> (from shell history, f'rinstance)
16:54 < noodles775> jtv: I just thought it would have been a simpler implementation, not more difficult (ie. LPCONFIG == 'development'), but yep, I'm 
                    updating the MP now.

review: Approve (code)

« Back to merge proposal