277502 - Re-organize checksetup.pl to be understood more easily (make it a short series of subroutines)

Status: RESOLVED FIXED

(Bugzilla :: Installation & Upgrading, enhancement, P3)

Description

[Max Kanat-Alexander]

The other morning I was thinking about how to make checksetup more logical and
easy to understand without risking major code changes (since checksetup is hard
to test when we change things).

Basically, I think that the basic code of checksetup.pl should look like the
classic main() function in an application, and just be a simple series of
subroutines. Something like:

check_versions()
check_schema()
create_admin()
check_sanity()

Or something like that. I'll have to investigate checksetup a bit more to get
the actual list of functions that it will consist of.

It will also make it a lot easier to futher improve the checksetup code, and
move some things out of that script that don't need to be there.

Comment 1

[Max Kanat-Alexander]

Attachment: Step-by-step "map" for checksetup.pl

I just spent a few hours making this. It's an English description,
step-by-step, of what checksetup does, in detail.

This should help a lot with any re-organization.

Comment 2

[Joel Peshkin]

Attchment 170860 looks like the right idea.  I suggest that each line (numbered
item) also become a subroutine as well as having a subroutine for each major
category.

Comment 3

[Max Kanat-Alexander]

(In reply to comment #2)
> Attchment 170860 looks like the right idea.  I suggest that each line (numbered
> item) also become a subroutine as well as having a subroutine for each major
> category.

  Exactly my plan. :-)

Comment 4

[Max Kanat-Alexander]

So, I'm thinking that as I do this, I might move these subroutines into perl
modules, and checksetup would just call them:

Bugzilla::Requirements
Bugzilla::Install

Bugzilla::Requirements would contain everything that's in the BEGIN block, now.
This would also allow other code (like runtests) to check that the modules exist.

Comment 5

[Max Kanat-Alexander]

I'm going to start some work on this in 2.22, but will probably not finish until
2.24.

Comment 6

[u197037]

And please, don't forget a routine (or, better, pluggable mechanism with
overrides/plug-ins) to check bz_schema against the real DB. Then we'll be able
to remove that wierd comment in localconfig for db_check parameter. I mean a
phrase about the "moon phase". :)
I've ran into it last week, so had to drop all the problem tables along with
bz_schema itself as a workaround. Thanks God, it was just a test DB.

Comment 7

[Max Kanat-Alexander]

(In reply to comment #6)
> And please, don't forget a routine (or, better, pluggable mechanism with
> overrides/plug-ins) to check bz_schema against the real DB. Then we'll be able
> to remove that wierd comment in localconfig for db_check parameter. I mean a
> phrase about the "moon phase". :)

  Hahahaha. Actually, that "moon phase" comment is totally unrelated to that
part of checksetup. The "moon phase" stuff is involved in checking the DB
version numbers and so forth.

> I've ran into it last week, so had to drop all the problem tables along with
> bz_schema itself as a workaround. Thanks God, it was just a test DB.

  That's hopefully also addressed by bug 299230.

Comment 8

[u197037]

(In reply to comment #7)
> (In reply to comment #6)
> > And please, don't forget a routine (or, better, pluggable mechanism with
> > overrides/plug-ins) to check bz_schema against the real DB. Then we'll be able
> > to remove that wierd comment in localconfig for db_check parameter. I mean a
> > phrase about the "moon phase". :)
> 
>   Hahahaha. Actually, that "moon phase" comment is totally unrelated to that
> part of checksetup. The "moon phase" stuff is involved in checking the DB
> version numbers and so forth.
Well, so we also have, say, "Venus phases" in there :)

> > I've ran into it last week, so had to drop all the problem tables along with
> > bz_schema itself as a workaround. Thanks God, it was just a test DB.
> 
>   That's hopefully also addressed by bug 299230.
No, that's not the case. I've been forced to drop the tables (or manually check
for correctness) to make their structure consistent with schema description.
Though I did not wish to.

Should I fill in the bug on this and add it into blockers of this one?

Comment 9

[Max Kanat-Alexander]

(In reply to comment #8)
> Should I fill in the bug on this and add it into blockers of this one?

  Well, actually, our whole discussion here is really unrelated to this bug. So
anything we're talking about should go into another bug. Actually *changing* the
way checksetup works would be a totally separate bug from this one, and
certainly any problems you're experiencing with checksetup would be a different
bug from this one. But yes, I'd certainly appreciate a bug report with the
details of the situation. :-)

Comment 10

[u197037]

Added bug 300452 in dependency list

Comment 11

[Max Kanat-Alexander]

Attachment: Patch: Final Cleanup

Okay, here's the very final step of this bug, this final cleanup of checksetup.pl.

You'll notice that there's now a very nice "How Checksetup Works" section in the POD. It should be useful to developers especially.

Granting myself review, as module owner.

Comment 12

[Max Kanat-Alexander]

Requesting approval directly, as module owner.

Comment 13

[Max Kanat-Alexander]

*Cheering can be heard!*

Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.543; previous revision: 1.542
done
Checking in Bugzilla/Install.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Install.pm,v  <--  Install.pm
new revision: 1.8; previous revision: 1.7
done
Checking in Bugzilla/Install/Requirements.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Install/Requirements.pm,v  <--  Requirements.pm
new revision: 1.18; previous revision: 1.17
done
Checking in template/en/default/global/messages.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/messages.html.tmpl,v  <--  messages.html.tmpl
new revision: 1.43; previous revision: 1.42
don