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

RESOLVED FIXED in Bugzilla 2.16

Status

()

Bugzilla
Installation & Upgrading
P1
blocker
RESOLVED FIXED
16 years ago
5 years ago

People

(Reporter: CodeMachine, Assigned: zach)

Tracking

2.15
Bugzilla 2.16

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

16 years ago
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.
(Reporter)

Comment 3

16 years ago
There would need a way to avoid updating $Id in specific circumstances (but not
by default), or the id would have no use.

Comment 4

16 years ago
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.
(Reporter)

Comment 5

16 years ago
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.
(Reporter)

Updated

16 years ago
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
(Assignee)

Updated

16 years ago
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
(Assignee)

Comment 9

16 years ago
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.

Comment 11

16 years ago
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.
(Reporter)

Comment 12

16 years ago
This should be about bumping the version number up on any change the
administrator might be interested in, not just API changes.
(Assignee)

Comment 13

16 years ago
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.
(Assignee)

Comment 14

16 years ago
Created attachment 59259 [details] [diff] [review]
Non-working first crack at this.

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.
(Assignee)

Comment 15

16 years ago
Created attachment 61035 [details] [diff] [review]
Patch v2, this should do the trick
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...
(Assignee)

Comment 17

16 years ago
Created attachment 61203 [details] [diff] [review]
v3
Attachment #61035 - Attachment is obsolete: true
(Assignee)

Comment 18

16 years ago
Created attachment 61204 [details] [diff] [review]
v3.1, without macbinary header
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
(Assignee)

Comment 23

16 years ago
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?
(Assignee)

Comment 25

16 years ago
Created attachment 69579 [details] [diff] [review]
v4

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
(Assignee)

Updated

16 years ago
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
(Assignee)

Comment 28

16 years ago
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.
(Reporter)

Comment 31

16 years ago
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
(Reporter)

Comment 34

16 years ago
Bueler?
(Assignee)

Comment 35

16 years ago
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?
(Reporter)

Comment 38

16 years ago
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.
(Assignee)

Comment 39

16 years ago
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?
(Assignee)

Comment 41

16 years ago
Created attachment 77284 [details] [diff] [review]
Add the version string to each template only and test to see that is is there

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+
(Assignee)

Comment 43

16 years ago
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-
(Assignee)

Comment 45

16 years ago
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
(Assignee)

Comment 47

16 years ago
Created attachment 77337 [details] [diff] [review]
Fix Gerv's points, change to bugzilla.org
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+
(Assignee)

Comment 50

16 years ago
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

Comment 52

16 years ago
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)
(Assignee)

Comment 53

16 years ago
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
Last Resolved: 16 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
(Reporter)

Comment 56

16 years ago
Where's the other half of this?

Comment 57

14 years ago
For reference, the other part if this is bug#186866

Comment 58

10 years ago
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.

Updated

10 years ago
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.