Re: Integrating a custom SCM / non-numeric change IDs
On Friday 15 May 2009 14:46:27 Christian Hammond wrote: > First, I want to be 100% sure that we're having the same definition of this > field. This field represents an upstream server-side ID that is tied to > that specific set of changes, and will exist up to and possibly past the > point where the change is committed, regardless of how many updates are > made to that review request. As I understand from the code, Perforce has a feature called changeset - a named entity with usual attributes of a commit: author, changes description etc, - which hasn't been integrated into development mainline. The possible difference between a real commit and a changeset is that commit is unchangeable, it's already published, while a changeset may be changed before it actually committed. Changesets are identified by changenums on Perforce side. In order to implement 'post-review' strategy I also need to refer a specific commit (with author, changes description, file diff) identified by its ID. > The upstream changeset information should > contain useful information, such as the description or bugs closed. Once a > changenum is set on a review request, it should not change. Well, since post-review approach works when a commit is already in the repository, there's no need to change this identifier in a corresponding review request -- all subsequent fixes if any are to be committed separately and can have separate review requests. Thus I think re-use of `changenum' field is possible, it serves quite similar purposes in case of Perforce "changesets" and commit/revision IDs. > So right now, raw SQL would be the only current option (assuming it can be > done in a standard way), but if we do it it must be thoroughly tested with > every database type Django supports. I would say that my preferred option > would be to patch django-evolution to support a ChangeFieldType that > converted between certain types, and then use that. That could be unit > tested and support could be tuned for each database type. Plus it's more > reusable and other projects could benefit. yes, I think it would be more reasonable to implement proper support in Django Evolution. But I don't have enough experience with all that code to solve the task myself. So I guess I won't implement proper data migration from numeric changenums to string change_ids right now. Alexey Morozov. --~--~-~--~~~---~--~~ You received this message because you are subscribed to the Google Groups "reviewboard" group. To post to this group, send email to reviewboard@googlegroups.com To unsubscribe from this group, send email to reviewboard+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/reviewboard?hl=en -~--~~~~--~~--~--~---
Re: Integrating a custom SCM / non-numeric change IDs
First, I want to be 100% sure that we're having the same definition of this
field. This field represents an upstream server-side ID that is tied to that
specific set of changes, and will exist up to and possibly past the point
where the change is committed, regardless of how many updates are made to
that review request. The upstream changeset information should contain
useful information, such as the description or bugs closed. Once a changenum
is set on a review request, it should not change.
To my knowledge, django-evolution has no way of changing a field's type to
an incompatible type. The FakeChangeFieldType works because the two types
are, at the database level, the same. Changing data types is a lot harder,
and there's 0 support for this in Django Evolution.
So right now, raw SQL would be the only current option (assuming it can be
done in a standard way), but if we do it it must be thoroughly tested with
every database type Django supports. I would say that my preferred option
would be to patch django-evolution to support a ChangeFieldType that
converted between certain types, and then use that. That could be unit
tested and support could be tuned for each database type. Plus it's more
reusable and other projects could benefit.
Christian
--
Christian Hammond - chip...@chipx86.com
Review Board - http://www.review-board.org
VMware, Inc. - http://www.vmware.com
On Thu, May 14, 2009 at 10:07 PM, Alexey Morozov wrote:
>
> Hi!
>
> On Tuesday 12 May 2009 04:41:06 Christian Hammond wrote:
>
> > As for the DB change, we have a nice database migration system in place.
> > You pretty much need to just make the change to the appropriate Model
> > class(es) and then add an Evolution definition (see
> reviews/evolutions/*.py
> > for example).
>
> Is it possible to express schema evolution in terms of django_evolution
> calls,
> not raw SQLs?
>
> As for model change, I think it would be smth like this:
>
> diff --git a/reviews/models.py b/reviews/models.py
> index 570551c..38e112b 100644
> --- a/reviews/models.py
> +++ b/reviews/models.py
> @@ -180,7 +180,7 @@ class ReviewRequest(models.Model):
> status = models.CharField(_("status"), max_length=1, choices=STATUSES,
> db_index=True)
> public = models.BooleanField(_("public"), default=False)
> -changenum = models.PositiveIntegerField(_("change number"),
> blank=True,
> +change_id = models.CharField(_("change ID"), max_length=255,
> blank=True,
> null=True, db_index=True)
> repository = models.ForeignKey(Repository,
>related_name="review_requests",
>
> but I'm not sure how existing RB DBs should be evolved. Seems that neither
> djblets' FakeChangeFieldType nor django_evolution's ChangeField isn't
> suitable for the task. Simply add a field and then write a raw SQL to copy
> data? Would it be portable enough? Any comments?
>
> Alexey Morozov
>
> >
>
--~--~-~--~~~---~--~~
You received this message because you are subscribed to the Google Groups
"reviewboard" group.
To post to this group, send email to reviewboard@googlegroups.com
To unsubscribe from this group, send email to
reviewboard+unsubscr...@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/reviewboard?hl=en
-~--~~~~--~~--~--~---
Re: Integrating a custom SCM / non-numeric change IDs
Hi!
On Tuesday 12 May 2009 04:41:06 Christian Hammond wrote:
> As for the DB change, we have a nice database migration system in place.
> You pretty much need to just make the change to the appropriate Model
> class(es) and then add an Evolution definition (see reviews/evolutions/*.py
> for example).
Is it possible to express schema evolution in terms of django_evolution calls,
not raw SQLs?
As for model change, I think it would be smth like this:
diff --git a/reviews/models.py b/reviews/models.py
index 570551c..38e112b 100644
--- a/reviews/models.py
+++ b/reviews/models.py
@@ -180,7 +180,7 @@ class ReviewRequest(models.Model):
status = models.CharField(_("status"), max_length=1, choices=STATUSES,
db_index=True)
public = models.BooleanField(_("public"), default=False)
-changenum = models.PositiveIntegerField(_("change number"), blank=True,
+change_id = models.CharField(_("change ID"), max_length=255, blank=True,
null=True, db_index=True)
repository = models.ForeignKey(Repository,
related_name="review_requests",
but I'm not sure how existing RB DBs should be evolved. Seems that neither
djblets' FakeChangeFieldType nor django_evolution's ChangeField isn't
suitable for the task. Simply add a field and then write a raw SQL to copy
data? Would it be portable enough? Any comments?
Alexey Morozov
--~--~-~--~~~---~--~~
You received this message because you are subscribed to the Google Groups
"reviewboard" group.
To post to this group, send email to reviewboard@googlegroups.com
To unsubscribe from this group, send email to
reviewboard+unsubscr...@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/reviewboard?hl=en
-~--~~~~--~~--~--~---
Re: Integrating a custom SCM / non-numeric change IDs
On Mon, May 11, 2009 at 2:32 PM, Thilo-Alexander Ginkel < thilo.gin...@gmail.com> wrote: > > On May 11, 9:55 pm, Christian Hammond wrote: > > With this SCM, is the change identifier a server-stored ID that contains > the > > description and other information for Review Board to parse? Or is it > more > > like an atomic ID representing that change that gets pushed to the server > > when committed? > > The SCM supports both committed and uncommitted changes and in both > cases the change ID provides a means to retrieve meta-information > about the change (such as a description, list of involved files, etc.) > from the SCM server. > > > If the former, and if you want to support pulling that information from > the > > server, then we'll need a patch to make Review Board less strict about > the > > type of input, and move validation of the ID into the SCMTools. > > > > [...] > > I actually got a first version to work by declaring the repository not > to support change sets and moving the summary / description retrieval > into post-review where the full change id is still available. > Nevertheless, it would be nice to be able to support this on the > server side leaving less hacks in post-review (and the positive side- > effect that submitting the same change a second time would update an > existing review). Short-term, post-review might be your best option. As for updating an existing review, you can use the -r flag. SVN, CVS, etc. users all have to use this anyway for updating their review requests. You just pass -r on the post-review command line. About patching RB to be less strict: I think the exception happens > somewhere in the relational database mapping, so changing this would > affect both the DB schema (which currently declares the changeset ID > as an int) as well as plenty other places in the code which I > currently cannot yet overlook being a novice to the RB code. Sounds like a bunch of work would be needed, but hopefully it's largely mechanical. As for the DB change, we have a nice database migration system in place. You pretty much need to just make the change to the appropriate Model class(es) and then add an Evolution definition (see reviews/evolutions/*.py for example). For a working development checkout and test site, you'd run: $ ./manage.py evolve --execute Or, if you install the new, modified Review Board, you can apply this to your site by running: $ rb-site upgrade /path/to/site The next time anyone does an upgrade, they'll get the updated definition. Christian -- Christian Hammond - chip...@chipx86.com Review Board - http://www.review-board.org VMware, Inc. - http://www.vmware.com --~--~-~--~~~---~--~~ You received this message because you are subscribed to the Google Groups "reviewboard" group. To post to this group, send email to reviewboard@googlegroups.com To unsubscribe from this group, send email to reviewboard+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/reviewboard?hl=en -~--~~~~--~~--~--~---
Re: Integrating a custom SCM / non-numeric change IDs
On May 11, 9:55 pm, Christian Hammond wrote: > With this SCM, is the change identifier a server-stored ID that contains the > description and other information for Review Board to parse? Or is it more > like an atomic ID representing that change that gets pushed to the server > when committed? The SCM supports both committed and uncommitted changes and in both cases the change ID provides a means to retrieve meta-information about the change (such as a description, list of involved files, etc.) from the SCM server. > If the former, and if you want to support pulling that information from the > server, then we'll need a patch to make Review Board less strict about the > type of input, and move validation of the ID into the SCMTools. > > [...] I actually got a first version to work by declaring the repository not to support change sets and moving the summary / description retrieval into post-review where the full change id is still available. Nevertheless, it would be nice to be able to support this on the server side leaving less hacks in post-review (and the positive side- effect that submitting the same change a second time would update an existing review). About patching RB to be less strict: I think the exception happens somewhere in the relational database mapping, so changing this would affect both the DB schema (which currently declares the changeset ID as an int) as well as plenty other places in the code which I currently cannot yet overlook being a novice to the RB code. Thanks, Thilo --~--~-~--~~~---~--~~ You received this message because you are subscribed to the Google Groups "reviewboard" group. To post to this group, send email to reviewboard@googlegroups.com To unsubscribe from this group, send email to reviewboard+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/reviewboard?hl=en -~--~~~~--~~--~--~---
Re: Integrating a custom SCM / non-numeric change IDs
With this SCM, is the change identifier a server-stored ID that contains the description and other information for Review Board to parse? Or is it more like an atomic ID representing that change that gets pushed to the server when committed? If the former, and if you want to support pulling that information from the server, then we'll need a patch to make Review Board less strict about the type of input, and move validation of the ID into the SCMTools. If the latter, then it's not really any different from our current SCMTools. The changeset field is really for specifying an ID of an uncommitted change that the SCM repository already knows about and has information useful for automatically pulling into the review request when created. Christian -- Christian Hammond - chip...@chipx86.com Review Board - http://www.review-board.org VMware, Inc. - http://www.vmware.com On Mon, May 11, 2009 at 4:09 AM, Thilo-Alexander Ginkel < thilo.gin...@gmail.com> wrote: > > Hello everybody, > > while attempting to add some basic support for a proprietary SCM to > Review Board / post-review I stumbled over one issue that I'm > currently uncertain how to solve: The SCM I'm integrating uses change > identifiers, which are not numeric. Instead arbitrary strings are > used, which (at least with alpha4) leads to the following exception > when attempting to create the review request: > - invalid literal for int() with base 10: '' > > Are there other SCMs supported by RB that also have to deal with this > issue? If so, how was the problem solved for these SCMs? > > Any input is highly appreciated! > > Thanks, > Thilo > > > --~--~-~--~~~---~--~~ You received this message because you are subscribed to the Google Groups "reviewboard" group. To post to this group, send email to reviewboard@googlegroups.com To unsubscribe from this group, send email to reviewboard+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/reviewboard?hl=en -~--~~~~--~~--~--~---
Integrating a custom SCM / non-numeric change IDs
Hello everybody, while attempting to add some basic support for a proprietary SCM to Review Board / post-review I stumbled over one issue that I'm currently uncertain how to solve: The SCM I'm integrating uses change identifiers, which are not numeric. Instead arbitrary strings are used, which (at least with alpha4) leads to the following exception when attempting to create the review request: - invalid literal for int() with base 10: '' Are there other SCMs supported by RB that also have to deal with this issue? If so, how was the problem solved for these SCMs? Any input is highly appreciated! Thanks, Thilo --~--~-~--~~~---~--~~ You received this message because you are subscribed to the Google Groups "reviewboard" group. To post to this group, send email to reviewboard@googlegroups.com To unsubscribe from this group, send email to reviewboard+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/reviewboard?hl=en -~--~~~~--~~--~--~---
