Merge lp://staging/~fgallina/rnr-server/clickpackagereview-architecture-to-arch_tag into lp://staging/rnr-server
Proposed by
Fabián Ezequiel Gallina
| Status: | Merged |
|---|---|
| Approved by: | Fabián Ezequiel Gallina |
| Approved revision: | 285 |
| Merged at revision: | 284 |
| Proposed branch: | lp://staging/~fgallina/rnr-server/clickpackagereview-architecture-to-arch_tag |
| Merge into: | lp://staging/rnr-server |
| Diff against target: |
568 lines (+398/-44) 8 files modified
src/clickreviews/forms.py (+3/-21) src/clickreviews/migrations/0003_auto__add_field_clickpackagereview_arch_tag.py (+133/-0) src/clickreviews/migrations/0004_populate_clickpackagereview_arch_tag.py (+132/-0) src/clickreviews/migrations/0005_auto__del_field_clickpackagereview_architecture.py (+127/-0) src/clickreviews/models.py (+1/-2) src/clickreviews/tests/factory.py (+2/-12) src/clickreviews/tests/test_forms.py (+0/-8) src/clickreviews/tests/test_handlers.py (+0/-1) |
| To merge this branch: | bzr merge lp://staging/~fgallina/rnr-server/clickpackagereview-architecture-to-arch_tag |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Matias Bordese (community) | Approve | ||
|
Review via email:
|
|||
Commit message
Remove dependency on `reviewsapp.
`clickreviews.
CharField sans additional validation.
Description of the change
.
To post a comment you must log in.
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
LGTM
(14:18:10) matiasb: fgallina: so... a few questions: why do we need this? (I guess we just save the value we get from the form without validation, and we are ok with that and don't need to keep architectures up to date here too?)
(14:18:10) matiasb: also, do you need keep the original field name? can't just add the new field with a different name?
(14:36:30) fgallina: matiasb: that's right, we want to save the value, do not validate for now. In the future possibly move to a API related validation but we are fine for now.
(14:37:12) fgallina: matiasb: as for the original name, it seemed like a good thing for consistency. It's a simple migration step, I could remove the rename if that's preferred.
(14:37:58) matiasb: fgallina: just checking if there was something else depending on the value, since now it will a different thing what you find there
(14:39:21) matiasb: fgallina: also, do you know how many rows would be affected by the data migration? we shouldn't do this kind of data (db expensive) migrations usually
(14:40:32) fgallina: matiasb: not really, for now it was not used anywhere other than just getting the value stored for a given review in clickreviews.
(14:41:03) fgallina: matiasb: no, but I won't expect to be *that* big, as this is just for reviews related to click apps. I'll get some numbers in a few.
(14:41:17) matiasb: ack, please, to be sure
(14:53:30) fgallina: matiasb: by parsing histogram data from stats, there are about 87704 reviews. No fun at all.
(14:53:41) fgallina: matiasb: I think the data migration can be optimized to use a single update statement (or separate chunks of apps to update via a single update)
(14:53:55) matiasb: fgallina: wow... all *click* reviews?
(14:53:58) nessita: fgallina: what we usually do in this cases is "on going" migration
(14:54:02) fgallina: matiasb: and we could leave this new field to be called arch_tag
(14:54:31) fgallina: matiasb, nessita: no wait.
(14:54:49) matiasb: an alternative is as nessita says, having a property that 'migrates' the value on the fly when needed (and we deprecate/rename the old field)
(14:54:53) fgallina: matiasb, nessita: too friday, I hit the old reviews endpoint.
(14:54:59) matiasb: :-)
(14:55:17) nessita: fgallina: stop napping!
(14:57:20) fgallina: matiasb, nessita: 6474 it is.
(14:58:06) matiasb: fgallina: ack, it doesn't sound terrible then, I guess
(14:58:10) nessita: fgallina: ack I guess is fine a data migration. For bigger number we need to do "migrate as we go:" approachs
(14:58:32) fgallina: matiasb, nessita: agreed.
(14:58:44) fgallina: matiasb: I think that either way it makes sense to keep the new field renamed.
(14:59:10) fgallina: matiasb: I feel arch_tag better describes what's expected to be found there, and matches the field name the API expected this whole time.
(14:59:31) matiasb: sounds good to me
(14:59:43) fgallina: matiasb: I'll rework that and let you know.