Closed Bug 274266 Opened 20 years ago Closed 19 years ago

Server type should not be hardcoded in SQL values

Categories

(Bugzilla :: Installation & Upgrading, defect)

2.19.2
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 2.20

People

(Reporter: goobix, Assigned: LpSolit)

References

Details

Attachments

(2 files, 1 obsolete file)

We plan to land PG-SQL, Sybase, etc support. The type of the SQL server should not be hard-coded in SQL configuration variables from checksetup.pl
Attached patch v1 (obsolete) (deleted) — Splinter Review
Assignee: zach → vladd
Status: NEW → ASSIGNED
Attachment #168533 - Flags: review?(bugzilla)
This probably isn't worth landing until we actually do support another database.
Depends on: bz-postgres
Why take the baby-steps approach? :-) Instead of waiting one big patch to land this, we can split it up and add progressive support for that. Smaller patches are easier to review and we'll reach there in shorter time.
This isn't a baby step. It's fixing something that's not broken (yet).
Vladd, I am trying to review this quickly - honest - but I'm looking at all the other places where mysql is mentioned in checksetup.pl, and considering the earlier comments from justdave as well... My first review request looked so easy at first.... I'll get it done tomorrow.
Comment on attachment 168533 [details] [diff] [review] v1 The changes in the patch are fine, and nice and simple, but I'm going to r- (sorry) because they are not really consistent within themselves (is it 'SQL database' or 'SQL server', or perhaps it should just be 'database'?). There are also some other places in checksetup.pl where 'MySQL' could be replaced to try and tidy it up which it would be nice to do at the same time -- if this was going to happpen. If you *really* wanted, then you could probably persuade me to r+ because this is a simple comment change which is not going to break anything, but from justdave's comments and dependency inddication, I doubt that you would get approval for this change anytime soon anyway?? (This could maybe be duped to bug#248175)
Attachment #168533 - Flags: review?(bugzilla) → review-
Assignee: vladd → nobody
Status: ASSIGNED → NEW
Attached patch patch, v2 (deleted) — Splinter Review
PostgreSQL is now supported. Most of the things reported in the previous patch have already been fixed.
Assignee: nobody → LpSolit
Attachment #168533 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #186011 - Flags: review?(mkanat)
Target Milestone: --- → Bugzilla 2.20
Attachment #186011 - Flags: review?(mkanat) → review+
Flags: approval?
Version: unspecified → 2.19.2
a=myk, although I would think this should be "database server".
Flags: approval? → approval+
Attached patch patch submitted (deleted) — Splinter Review
replace 'SQL server' by 'database server'. This is the patch I commited.
Checking in checksetup.pl; /cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v <-- checksetup.pl new revision: 1.404; previous revision: 1.403 done
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: