Closed Bug 98658 Opened 23 years ago Closed 22 years ago

Add version headers to all templates in preparation for later admin notification of template changes.

Categories

(Bugzilla :: Installation & Upgrading, defect, P1)

2.15
defect

Tracking

()

RESOLVED FIXED
Bugzilla 2.16

People

(Reporter: CodeMachine, Assigned: zach)

References

Details

Attachments

(1 file, 6 obsolete files)

If an administrator has a customised template in the custom directory, and the
template in the default directory is modified by the Bugzilla team, we should
notify the administrator.

The changes might be small, or they might cause incompatibility.  It might be
required, possibly required or beneficial for the admin to update their custom
template.

One implementation is for checksetup.pl to maintain a list of CRCs for all the
templates and regenerate and check them each run.  This however would make it
impossible to make changes that are "below-the-radar" such as license/comment
changes, unless we added a preprocessing step to strip these out.

An alternative option is to store version numbers, but any system where these
needed to be manually updated would be asking for trouble.
-> Installation
Assignee: justdave → zach
Component: Administration → Installation
As long as there's a way to mask it so the template system doesn't trip out on
it, you can place an $Id$ line anywhere in the text of a file, and the cvs
system will automatically replace it with the filename, cvs revision number, who
last changed it, and the date&time of the checkin whenever anyone checks the
file out.
There would need a way to avoid updating $Id in specific circumstances (but not
by default), or the id would have no use.
Matty, $Id$ can't be "not updated" on checkin as it tells who made a checkin,
what the current revision is (which always changes) and the date/time of the
checkin.  If that makes it unusable in this context, then so does any other use
of the revision number.

That being said, we should be able to add the following line to all the templates:
<!-- $Id$ -->

That, of course, assumes that Template Toolkit recognizes <!-- --> as a comment.
My logic was CRC would be easier, but that might not be the case if this stuff
is automatic.

CRC would also work regardless of CVS, but I'm not sure how the default
templates would be edited outside that environment.
The Template Toolkit does not recognize <!-- --> as a comment, although it could
probably be configured to do so.  It does, however pass <!-- --> through to the
template output, so you can use that tag when you want to insert an HTML comment
into the generated output.  Otherwise TT recognizes [%# %] as a TT comment and
will not do anything with its contents.
Priority: -- → P2
Target Milestone: --- → Bugzilla 2.16
This is going to be a blocker for the 2.16 release
Severity: enhancement → blocker
Priority: P2 → P1
Status: NEW → ASSIGNED
It seems to me that the majority of changes made to the default templates will
not be incompatible - HTML fixes, cosmetic changes, whatever. I can't see any
way that we can get some automated checker to detect which changes are relevant,
unless it's extremely intelligent.

It seems to me that the best fix for this is for each template to say, at the
top of it, what its interface is - which variables it expects to be passed in.
If someone changes that interface, they will also need to update the comment.
Even aside from this requirement, such documentation is desireable.

We can then enclose the comment in some detectable tags, and CRC the space
between them. Or, we can just look at the differences manually just before a
release.

Gerv
Why not use a UUID-like system somewhat like XPCOM. Each template 
gets a UUID and that UUID changes when the template API changes. A 
custom template would have the same UUID as the default template it is 
based on, just with a different domain. checksetup.pl could then see if any 
custom templates are installed, and if so examine the UUID's to 
determine if things have been changed and print messages to the user 
telling them what to do.

UUID's would be in a comment and look like (for a default template):
<!-- e280371e-1dd1-11b2-a7d0-81a8d463ab35@mozilla.org -->

If redhat (for example) wanted to customize the template, it would be the 
same UUID, just with @redhat.com or @bugzilla.redhat.com at the end. 

We could also use version numbers, but people may make the mistake of 
bumping up the version number when they don't change the API. 
Generating the uuid is easy: mozbot uuid will get you one.

This is currently targeted at 2.16 as a blocker. We need a spec _now_ for 
how this will work if we want it in by the end of the month.

Zach
>We could also use version numbers, but people may make the mistake of 
>bumping up the version number when they don't change the API. 
>Generating the uuid is easy: mozbot uuid will get you one.

Making sure version increments are done correctly should become part of the
review process.
I think a simple version number would probably work for bugzilla. Making things
easier for people to manage and read is good; otherwise we end up with things
like "what API was your template designed against, @#$&#@*($&!@#!@ or
@##&@#$(*#&?" instead of "what API version is your template, v1 or v3?" IMHO
simplicity would win out here.

So my proposal is: make a standard heading for templates that includes what API
version it expects, and make the .cgi file include a standard heading stating
the API version it exports and what variables it assumes.
This should be about bumping the version number up on any change the
administrator might be interested in, not just API changes.
let's do the best of both worlds: a version number and a domain. The 
domain won't be used now, but could be used later on. 

Every template would begin with:
<html>
<head>
<!-- 1.0@mozilla.org -->
etc...

If (and when) we do a skined thing for bugzilla so users can select skins, 
we could have: 


<!-- 1.0@kidprint.mozilla.org -->

All templates would start at 1.0 now and numbers would be bumped up 
as part of review. Someone needs to document this, barnboy? 

What do you think? Simplicity of version numbers and the expandibility of 
domains for later.
Attached patch Non-working first crack at this. (obsolete) — Splinter Review
This does not yet work, but if the logic is fixed in checksetup.pl it should be
ok. I'll get to this in a day or two unless someone beats me to it.
Attachment #59259 - Attachment is obsolete: true
Comment on attachment 61035 [details] [diff] [review]
Patch v2, this should do the trick

it's a minor nit-pick, but how about saying "out of sync" instead of "out of
date" since your giving the error if the version doesn't match, whether or not
it's greater or less...
Attached patch v3 (obsolete) — Splinter Review
Attachment #61035 - Attachment is obsolete: true
Attached patch v3.1, without macbinary header (obsolete) — Splinter Review
Attachment #61203 - Attachment is obsolete: true
Comment on attachment 61204 [details] [diff] [review]
v3.1, without macbinary header

Uncaught exception from user code:
	Uncaught exception from user code:
	Uncaught exception from user code:
	Uncaught exception from user code:
	Can't locate Support/Files.pm in @INC (@INC contains:
/sw/lib/perl5/darwin /sw/lib/perl5 /System/Library/Perl/darwin
/System/Library/Perl /Library/Perl/darwin /Library/Perl /Library/Perl
/Network/Library/Perl/darwin /Network/Library/Perl /Network/Library/Perl .) at
t/Support/Templates.pm line 25.
	Support::Templates::BEGIN() called at Support/Files.pm line 25
	require 0 called at Support/Files.pm line 25
	require t/Support/Templates.pm called at ./checksetup.pl line 245
	main::BEGIN() called at Support/Files.pm line 25
	require 0 called at Support/Files.pm line 25
BEGIN failed--compilation aborted at t/Support/Templates.pm line 25.
	require t/Support/Templates.pm called at ./checksetup.pl line 245
	main::BEGIN() called at t/Support/Templates.pm line 25
	require 0 called at t/Support/Templates.pm line 25
Compilation failed in require at t/Support/Templates.pm line 245.
	main::BEGIN() called at t/Support/Templates.pm line 245
	require 0 called at t/Support/Templates.pm line 245
BEGIN failed--compilation aborted at ./checksetup.pl line 245.
Attachment #61204 - Flags: review-
To fix attachment 61204 [details] [diff] [review] change this line:

use t::Support::Templates;

to this:

require "./t/Support/Templates.pm";

or these:

use lib "t";
use Support::Templates;
The error message should present the dodgy files as a list at the end of the
process, not print four lines for every template.

Also, presumably 
+my @templates = @Support::Templates::testitems;
doesn't, in fact, get a list of the custom templates a user has. Does it?

Should we be doing sanity checks on the domain name part? If not, why's it
there?

Also, the standard templates should have a domain. Thinking about it, why aren't
we using contract ID syntax? This seems designed for this sort of thing.

Gerv
zach: ping? :-)

Gerv
gerv, I'm a little confused about your comments, when you have a sec can 
you get on irc?

Zach
zach, gerv, ping...

have you two gotten together and figured this out yet?
Attached patch v4 (obsolete) — Splinter Review
I didn't get a chance to talk to gerv, but I think a bunch of us figured it out
on irc.
Attachment #61204 - Attachment is obsolete: true
Keywords: patch, review
Comment on attachment 69579 [details] [diff] [review]
v4

1) cp template/default/global/header template/custom/global/header
2) ./checksetup.pl
(no mismatch found, version should match anyway)
3) change mozilla.org to syndicomm.com in the version header
4) ./checksetup.pl
no mismatch found, so far so good
5) change 1.0 to 1.1 in the version header
6) ./checksetup.pl
no mismatch found.  maybe ok, it's newer than the default one
7) change 1.1 to 0.9 in the version header
8) ./checksetup.pl
no mismatch found.  this is a problem
Attachment #69579 - Flags: review-
Comment on attachment 69579 [details] [diff] [review]
v4

>+print "Checking custom template versions\n";
>+use lib "t";

Is this the testing library? Should we be requiring it for normal Bugzilla
operation?

>+use Support::Templates;
>+my @templates = @Support::Templates::testitems;

And what about this? Shouldn't we be using a recursive glob?

>+   if (-e "template/custom/".$file) { # if there is a custom version...

Nit: "template/custom/" . $file

>+      $currenttemplate =~ /\[\%\#<!--\s(.*?)\@.*?\s-->/; # get the version number in $1

Why do you require HTML comments around template version numbers? E.g.
>+[%#<!-- 1.0@mozilla.org -->%]

What's wrong with:

>+[%# Version: 1.0@bugzilla.org %]

(We should be using bugzilla.org, probably.)

>+      if ($customversion ne $defaultversion) {

We should do something more sophisticated than this, IMO. It would be nice to
have "harmless" 
warnings which say "We've put extra cool stuff in this template - fix it if you
like, but it's not
vital", as opposed to "your template will break if you don't fix this."

This means the numbering scheme probably requires a bit more thought. But this
must be a problem 
that's been solved before.

In your next patch, only add headers to a couple of template files (for
testing) - you're chasing a moving target, and it's not worth it.

Gerv
I don't mind chasing a moving target, most of the 2.16 templates are done 
and I can always add more later. Quick responces to your comments:

1. Testing library/recursive glob:
The testing library currently does some things that we don't want to do (it 
won't find included templates like header and footer plus we probably 
shouldn't use the tests anyway. I'll have this fixed in the next patch.

2. Why html comments?:
I guess there isn't much reason for this. I'll cut it out of the next version. 

3. More advanced system:
How about this:
An increase in the major version number (before the decimal point) 
indicates a major change that will break all old templates. An increase in 
the minor version number (after the decimal point) indicates a minor 
change (though typo fixing, etc shouldn't get an increase at all IMHO) that 
old templates will still work with. 

4. bugzilla.org for domains:
Good idea, that way mozilla.org can use custom templates. 

Let me know what you think about this. I'll wait before going forward with a 
new patch.

Zach
Your comment on 3) sounds fine. Make sure the comparison allows 10.1 to be > 9.3
and 10.12 to be greater than 10.9 . Should we pad the decimal part with a zero
(1.00, 2.06)?

Gerv
What about between releases? Can we make a policy that templates version numbers
are only bumped once per stable release? Else we're going to end up at version
100 real easily. Not to mention the cvs conflicts that it would likely cause.
If we bump version numbers once, we are going to potentially break
administrators running off CVS.  I don't see any problem with bumping by 100
each release.
Keywords: patch, review
Zach: ping, what's your status on this?
Zach?

Gerv
Bueler?
I didn't die, working on this now
We don't need this for 2.16, since 2.16 is the first official release containing
templates and thus does not contain any templates that have been modified from
previously released versions.  Reducing severity accordingly.
Severity: blocker → normal
Myk, I think there is one part of this that is indeed a blocker: If we don't put 
any kind of version information in the templates that are shipped with 2.16, how 
 can it be possible to solve this for 2.18? Well... maybe you are proposing to 
treat all versionless templates as always outdated? But then you can't treat 
missing version information as an error condition any more... Can you clarify?
Yes this was marked as a blocker for 2.16 because you need the information for
2.16 to know its outdated for 2.18.
Why don't we do this in two stages. I'll fix the patch up to include only the 
version information (starting at 1.0) and get that in. The checksetup.pl 
logic part can get in if it's ready by RC, or we can hold off on it until after 
release.
How far away are you from getting the version info into the templates?
This patch will add the version string to every template and test each template
to ensure that it is present as part of the tests. I did the version string as
an html comment so that the user will be able to see what version is being sent
for debugging if need be.
Attachment #69579 - Attachment is obsolete: true
Comment on attachment 77284 [details] [diff] [review]
Add the version string to each template only and test to see that is is there

The patch failed with a "malformed patch" error on line 93, which turned out to
be a typo on line 89 ("+1,3" instead of "+1,4").  With that typo fixed the
patch applies fine, the tests complete successfully, and templates work. r=myk
Attachment #77284 - Flags: review+
The malformed patch error thing is my fault, I manually changed 
something in the patch and forgot I couldn't do that. Someone please 2x 
review this.
Keywords: review
Comment on attachment 77284 [details] [diff] [review]
Add the version string to each template only and test to see that is is there

> if ($firstline =~ /<!-- \d\.\d+\@\w+\.\w+ -->/) {

This fails to match e.g.
1.0@bugzilla.gerv.net. 
Or 10.0@mozilla.org, for that matter.
Does \w correspond exactly to the characters valid in a hostname?

Also, I'm sure we were going to use bugzilla.org rather than mozilla.org for
our templates...

Gerv
Attachment #77284 - Flags: review-
New patch will be up by 4 this afternoon
This is still a blocker until the first half of the fix gets checked in.
Severity: normal → blocker
Attachment #77284 - Attachment is obsolete: true
Comment on attachment 77337 [details] [diff] [review]
Fix Gerv's points, change to bugzilla.org

r= justdave

passes tests, takes all manner of wonky version strings I tried throwing at it.

But get rid of the tabs you added to 004template.t before you check it in :)
Attachment #77337 - Flags: review+
Comment on attachment 77337 [details] [diff] [review]
Fix Gerv's points, change to bugzilla.org

Works for me.  r=myk
Attachment #77337 - Flags: review+
stage 1 commited, keeping this open to track the second part of this bug 
(adding the logic to checksetup)
Correcting the milestone now that part one has been committed.
Target Milestone: Bugzilla 2.16 → Bugzilla 2.18
Out of curiosity, wouldn't it be better to use the RCSid as the version numer,
as that's guranteed to change whenever a template gets updated. There appear to
be a few files that havn't had their version updated over several revisions in
CVS. This would make it extreemly hard for an admin to pickup changes in files.

Also, another advantage of using RCSids is that when I do an update, I can use
cvs diff to find out what's changed in a template since the last time I synced
it, without having to guess what has changed.
(our site tends to follow the CVS version of Bugzilla)
We considered using the RCS version string, however, doing that means 
that the version number _always_ is increased, and there are times when 
we don't want to do that. Also, we want to be able to bump the major 
version string ( the 1 in 1.0) for big changes and the minor string (the .0 in 
1.0) for little quick changes. You can't do that with RCS versions.
You can up the major verison, but its a pain, and I don't know if bonsai handles
handles it.
This already has a checkin, I want it off the "ready to check in" list since
it's not and/or already has been.  Let's do a new bug to finish the job.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Summary: Let administrator know which customised templates have been updated by Bugzilla team. → Add version headers to all templates in preparation for later admin notification of template changes.
Target Milestone: Bugzilla 2.18 → Bugzilla 2.16
Where's the other half of this?
For reference, the other part if this is bug#186866
Do you realize that this bug is the most useless "blocker/P1 ultra important" bug ever filed? :) 5 years later, all templates still mention 1.0@bugzilla.org. Back this out! And I'm not kidding.
Blocks: 392186
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

Created:
Updated:
Size: