RANT: r1999 breaks production systems
Jorge Godoy
Gentlemen,
please, lets be more careful with commits on the *stable* branch. I don't
think that changing the identity code like it was done is right since there
are some differences between the model that was used and the one available at
model.py, specially with regards to table names.
I don't consider this a critical bug that needed being fixed before releasing
1.0 and breaking applications that went into production.
(And I just got to know that because a disk failed on a server and I
reinstalled TG and updated the 1.0 branch there.)
Be seeing you,
--
Jorge Godoy <jgo...@gmail.com>
Lee McFadden
What is the difference between the two models? I have just gone over
the code and I can safely switch my test apps between the old
ActiveMapper models and the straight SA models without any problems at
all. All the table names are the same in the database and identity
works perfectly.
Obviously if there's something that I'm missing I'd like to know as
I'm the one that committed the code and would like to make sure that
it's done properly :)
Lee
--
Lee McFadden
blog: http://www.splee.co.uk
work: http://fireflisystems.com
Jorge Godoy
> Jorge,
>
> What is the difference between the two models? I have just gone over
> the code and I can safely switch my test apps between the old
> ActiveMapper models and the straight SA models without any problems at
> all. All the table names are the same in the database and identity
> works perfectly.
>
> Obviously if there's something that I'm missing I'd like to know as
> I'm the one that committed the code and would like to make sure that
> it's done properly :)
r1999 affects sovisit.py, so it should only affect SQL Object related
projects.
--
Jorge Godoy <jgo...@gmail.com>
Lee McFadden
> projects.
>
Ah, mine was the revision before... my memory is playing tricks on
me. Ignore me :)
Jorge Vargas
>
> please, lets be more careful with commits on the *stable* branch. I don't
> think that changing the identity code like it was done is right since there
> are some differences between the model that was used and the one available at
> model.py, specially with regards to table names.
not supposed to
is there anything different then the table name?
http://trac.turbogears.org/turbogears/browser/branches/1.0/turbogears/visit/sovisit.py
81 class TG_Visit(SQLObject):
82 class sqlmeta:
83 table="tg_visit"
84
85 visit_key= StringCol( length=40, alternateID=True,
86 alternateMethodName="by_visit_key" )
87 created= DateTimeCol( default=datetime.now )
88 expiry= DateTimeCol()
89
90 def lookup_visit( cls, visit_key ):
91 try:
92 return cls.by_visit_key( visit_key )
93 except SQLObjectNotFound:
94 return None
95 lookup_visit= classmethod(lookup_visit)
http://trac.turbogears.org/turbogears/browser/branches/1.0/turbogears/qstemplates/quickstart/%2Bpackage%2B/model.py_tmpl
29 class Visit(SQLObject):
30 class sqlmeta:
31 table = "visit"
32
33 visit_key = StringCol(length=40, alternateID=True,
34 alternateMethodName="by_visit_key")
35 created = DateTimeCol(default=datetime.now)
36 expiry = DateTimeCol()
37
38 def lookup_visit(cls, visit_key):
39 try:
40 return cls.by_visit_key(visit_key)
41 except SQLObjectNotFound:
42 return None
43 lookup_visit = classmethod(lookup_visit)
>
> I don't consider this a critical bug that needed being fixed before releasing
> 1.0 and breaking applications that went into production.
>
actually this fix a bug that I think your code was depending on that
was introduced in
http://trac.turbogears.org/turbogears/changeset/1604#file12
as you can see Kevin moved the sovisit code to check for a config
entry and added the Visit table to model.py
but didn't changed the variable so people changing their visit class
in model.py where not getting anything out of it.
> (And I just got to know that because a disk failed on a server and I
> reinstalled TG and updated the 1.0 branch there.)
>
Anyway I'm sorry I didn't knew you where running out of a 1.0 co I'll
be more careful.
Jorge Godoy
> On 10/23/06, Jorge Godoy <jgo...@gmail.com> wrote:
>>
>>
>> Gentlemen,
>>
>> please, lets be more careful with commits on the *stable* branch. I don't
>> think that changing the identity code like it was done is right since there
>> are some differences between the model that was used and the one available at
>> model.py, specially with regards to table names.
> I'm sorry I should had check if those models where different. they are
> not supposed to
>
> is there anything different then the table name?
Yes. There are different fields on model.py and in the class you "removed"
from the code.
Check the code in turbogears.visit.sovisit.TG_Visit and at the model in a
quickstarted project.
http://trac.turbogears.org/turbogears/browser/branches/1.0/turbogears/visit/sovisit.py#L81
and
Check that there's "Visit" and "VisitIdentity" and in old code there was even
another differente where the name for VisitIdentity's table was
'tg_visit_identity' and not the default name supplied by SQL Object (and I
believe that in a prior discussion it was agreed that the prefix 'tg_' was a
good thing to have and to keep since it allowed the project to use similar
names without clashing with TG's support tables).
> actually this fix a bug that I think your code was depending on that
> was introduced in
>
> http://trac.turbogears.org/turbogears/changeset/1604#file12
I found this bug because after the update I couldn't log into the system.
This was an identity failure, not on my code. So I believe this was a bug in
identity. It hadn't reached my code yet.
> as you can see Kevin moved the sovisit code to check for a config
> entry and added the Visit table to model.py
I saw that.
> but didn't changed the variable so people changing their visit class
> in model.py where not getting anything out of it.
I know. But there were these differences that aren't clear enough in a
*stable* migration path. If it was in the trunk, I'd think that this was a
good change, but in a stable branch I believe it will cause some harm.
> Anyway I'm sorry I didn't knew you where running out of a 1.0 co I'll
> be more careful.
That's the only way I found where I could intensively test new features. I'm
glad I got this now and not after releasing something new.
So, we have to check this migration path and document the necessary changes
for projects quickstarted with 0.9.x / 1.0b1 to what will be the next 1.0
release.
The rant was not because I'm running from 1.0, but because this change messed
with more things than it should without some big warning and/or migration
path.
Providing the information on how to upgrade looks like the best fix to me.
--
Jorge Godoy <jgo...@gmail.com>
Jorge Godoy
> Providing the information on how to upgrade looks like the best fix to me.
Here's what need to be changed / checked:
1. Make sure that in app.cfg there's a line like this one:
visit.soprovider.model = "${package}.model.Visit"
2. Change class VisitIdentity in model.py removing the metaclass
sqlmeta and the alternate table name
3. Run 'tg-admin sql create'
4. If you need to keep some kind of history, then it is interesting to
run the followin SQL command
insert into visit_identity (id, visit_key, user_id)
select id, visit_key, user_id from tg_visit_identity;
so that your data gets migrated from one table to the other
5. Remove the old table (SQL again):
drop table tg_visit_identity;
And sorry if my messages sounded more rude than they should. I should have
provided this "doc patch" before.
--
Jorge Godoy <jgo...@gmail.com>
Jorge Vargas
>
> "Jorge Vargas" <jorge....@gmail.com> writes:
>
> > On 10/23/06, Jorge Godoy <jgo...@gmail.com> wrote:
> >>
> >>
> >> Gentlemen,
> >>
> >> please, lets be more careful with commits on the *stable* branch. I don't
> >> think that changing the identity code like it was done is right since there
> >> are some differences between the model that was used and the one available at
> >> model.py, specially with regards to table names.
> > I'm sorry I should had check if those models where different. they are
> > not supposed to
> >
> > is there anything different then the table name?
>
> Yes. There are different fields on model.py and in the class you "removed"
> from the code.
>
> Check the code in turbogears.visit.sovisit.TG_Visit and at the model in a
> quickstarted project.
>
> http://trac.turbogears.org/turbogears/browser/branches/1.0/turbogears/visit/sovisit.py#L81
>
> and
> http://trac.turbogears.org/turbogears/browser/branches/1.0/turbogears/qstemplates/quickstart/%2Bpackage%2B/model.py_tmpl?rev=1978#L32
>
aren't those 2 the snip I pasted?
>
> Check that there's "Visit" and "VisitIdentity" and in old code there was even
> another differente where the name for VisitIdentity's table was
> 'tg_visit_identity' and not the default name supplied by SQL Object (and I
> believe that in a prior discussion it was agreed that the prefix 'tg_' was a
> good thing to have and to keep since it allowed the project to use similar
> names without clashing with TG's support tables).
>
> > actually this fix a bug that I think your code was depending on that
> > was introduced in
> >
> > http://trac.turbogears.org/turbogears/changeset/1604#file12
>
> I found this bug because after the update I couldn't log into the system.
> This was an identity failure, not on my code. So I believe this was a bug in
> identity. It hadn't reached my code yet.
I believe what happen here is that before you upgraded identity loaded
both your tg_visit and visit ttables then set the visit table as
default BUT keep using the tg_visit, after the upgrade since now it
goes to the table it should (the one specified in the config) it's
going to the visit table which is empty! together with the
visit_identity table.
> I know. But there were these differences that aren't clear enough in a
> *stable* migration path. If it was in the trunk, I'd think that this was a
> good change, but in a stable branch I believe it will cause some harm.
>
comment out the line that uses the model.py classes?
> > Anyway I'm sorry I didn't knew you where running out of a 1.0 co I'll
> > be more careful.
>
> That's the only way I found where I could intensively test new features. I'm
> glad I got this now and not after releasing something new.
>
that's a good idea.
Jorge Vargas
>
> > Providing the information on how to upgrade looks like the best fix to me.
>
> Here's what need to be changed / checked:
>
> 1. Make sure that in app.cfg there's a line like this one:
>
> visit.soprovider.model = "${package}.model.Visit"
>
> 2. Change class VisitIdentity in model.py removing the metaclass
> sqlmeta and the alternate table name
>
http://trac.turbogears.org/turbogears/browser/branches/1.0/turbogears/qstemplates/quickstart/%2Bpackage%2B/model.py_tmpl?rev=1998#L48
> 3. Run 'tg-admin sql create'
>
> 4. If you need to keep some kind of history, then it is interesting to
> run the followin SQL command
>
> insert into visit_identity (id, visit_key, user_id)
> select id, visit_key, user_id from tg_visit_identity;
>
> so that your data gets migrated from one table to the other
>
> 5. Remove the old table (SQL again):
>
> drop table tg_visit_identity;
>
just out of curiosity commenting out this line
visit.soprovider.model = "${package}.model.Visit"
should revert you back to using the tables inside the model this is an
"alternative" migration path.
>
>
> And sorry if my messages sounded more rude than they should. I should have
> provided this "doc patch" before.
>
np that's why this is opensource so we could point out other people's
mistakes and fix them for a better software :)
Jorge Godoy
> there is no meta in that class
You're considering a quick started app. I'm talking about evoving from a
*stable* release to another *stable* release without a big change in version
numbering (i.e. 1.0b1 to 1.0b2 and not 1.0 to 1.1 or 1.0 to 2.0). Things
should be clearer and more stable here. But that's just my opinion. :-) As
responsible for your code inside TG your opinion may differ (and it is good
that it does, since discussing these things make us have a better product).
> just out of curiosity commenting out this line
> visit.soprovider.model = "${package}.model.Visit"
>
> should revert you back to using the tables inside the model this is an
> "alternative" migration path.
Indeed. So this should also go into the documentation.
> np that's why this is opensource so we could point out other people's
> mistakes and fix them for a better software :)
That was my intention. :-) Maybe my wording was too harsh, and that was not
the intention. :-)
--
Jorge Godoy <jgo...@gmail.com>
Kevin Dangoor
>
> "Jorge Vargas" <jorge....@gmail.com> writes:
>
>> there is no meta in that class
>
> You're considering a quick started app. I'm talking about evoving
> from a
> *stable* release to another *stable* release without a big change
> in version
> numbering (i.e. 1.0b1 to 1.0b2 and not 1.0 to 1.1 or 1.0 to 2.0).
> Things
> should be clearer and more stable here. But that's just my
> opinion. :-) As
> responsible for your code inside TG your opinion may differ (and it
> is good
> hat it does, since discussing these things make us have a better
> product).
Yes, you are correct. 1.0 is definitely at a "stable" point and we
shouldn't be making breaking API changes there unless there's a
critical bug and no other choice.
Kevin