Save patches dropped by filtering #102
Conversation
Pull Request Test Coverage Report for Build 322
💛 - Coveralls |
There was a problem hiding this comment.
OMG, I just read the whole thing with generating the series ID to store in the patches table. Argh.
Can we just rip that out first and then do this change?
It doesn't make sense to just pull out the series_id out of our a** and store it, and nothing is using it anyway.
I.e. remove the series_id column (from new databases), and remove all code adding it (it will just add as empty string to existing tables, I tested that).
Then hopefully the code will be simpler. What do you think?
|
BTW, thanks for pulling this through, the code must have been a pain to figure out. Perhaps we should reduce our further suffering by just removing the series_id storage. |
|
@spbnick yeah, that's related to issue #80. I still need to work on that one and it's pretty hard to unwind all the parts (wouldn't be opposed to getting more help), but since this seemed like a higher priority task to fix, I did this first (since the DB one would take a loong time). Can you add your comment about getting rid of series ID to that issue, so we can track it properly where it belongs, or even submit a pull for that change? If you are too busy to do the change on top of this patch, just copy the comment and I'll enhance this pull next week. |
It's not used anywhere and totally useless since for PW1 it's created on the fly and not representing anything. Signed-off-by: Veronika Kabatova <vkabatov@redhat.com>
veruu
force-pushed
the
db
branch
3 times, most recently
from
3ce48e6 to
80bb2af
Compare
|
@spbnick added the |
sktm/__init__.py
Outdated
| interface: Interface of the Patchwork project the patch belongs to. | ||
| patch_url: URL of the patch to retrieve info tuple for. | ||
|
|
||
| Returns: Tuple of the patch info tuple (patch_id, patch_name, |
There was a problem hiding this comment.
This piece got a bit stuck in the past, I assume. Not required for my ACK.
sktm/__init__.py
Outdated
| """ | ||
| match = re.match(r'(.*)/patch/(\d+)$', patch_url) | ||
| if not match: | ||
| raise Exception('Malfomed patch url: %s' % patch_url) |
There was a problem hiding this comment.
Typo, should be "Malformed". Not required for my ACK.
Dropped patches are always rechecked if no new patch passed the filter script. Instead, save them as seen so they aren't checked all over again each time. Extract retrieving the info tuple into separate method (get_patch_info_from_url), which can be then used in both the original place (saving tested patches) and when saving the dropped patches. Fixes cki-project#100 Signed-off-by: Veronika Kabatova <vkabatov@redhat.com>
We have real migration scripts now, no need to keep the file. Signed-off-by: Veronika Kabatova <vkabatov@redhat.com>
|
Ah, two comments on commit organization, which we can improve in future PRs:
|
|
We already had one migration in place (yeah, the placeholder should have been removed with that one but it wasn't). agree with the second comment and will try to remember it for the future! |
Dropped patches are always rechecked if no new patch passed the filter
script. Instead, save them as seen so they aren't checked all over again
each time.
Extract retrieving the info tuple and series IDs into separate method
(get_patch_info_from_url), which can be then used in both the original
place (saving tested patches) and when saving the dropped patches.
Fixes #100
Signed-off-by: Veronika Kabatova vkabatov@redhat.com