Skip to content

Process diffs only once in load_in_patch#398

Merged
La0 merged 3 commits intomozilla:masterfrom
La0:fix-in-patch
Jan 7, 2020
Merged

Process diffs only once in load_in_patch#398
La0 merged 3 commits intomozilla:masterfrom
La0:fix-in-patch

Conversation

@La0
Copy link
Copy Markdown
Collaborator

@La0 La0 commented Jan 7, 2020

Found 2 bugs when running on Heroku testing:

  1. weirdly i get a decode exception using parsepatch there
  2. saw a lot of diffs processed multiple times, my sql request lacked a DISTINCT

@La0 La0 added the bug Something isn't working label Jan 7, 2020
@La0 La0 requested a review from marco-c January 7, 2020 11:12
@La0 La0 self-assigned this Jan 7, 2020
@marco-c
Copy link
Copy Markdown
Collaborator

marco-c commented Jan 7, 2020

weirdly i get a decode exception using parsepatch there

That's strange, could you file an issue on https://github.com/mozilla/pyo3-parsepatch/issues, with the example case where it is failing.

@La0
Copy link
Copy Markdown
Collaborator Author

La0 commented Jan 7, 2020

Comment thread backend/code_review_backend/issues/management/commands/load_in_patch.py Outdated
@marco-c
Copy link
Copy Markdown
Collaborator

marco-c commented Jan 7, 2020

Could you split the PR in two, since they are two separate bug fixes not depending on each other?

Bastien Abadie and others added 2 commits January 7, 2020 12:34
…_patch.py

Co-Authored-By: Marco Castelluccio <mcastelluccio@mozilla.com>
@La0
Copy link
Copy Markdown
Collaborator Author

La0 commented Jan 7, 2020

Ok, this one only has the DISTINCT

Copy link
Copy Markdown
Collaborator

@marco-c marco-c left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, please fix the PR name / commit message when landing

@La0 La0 merged commit 27e4b92 into mozilla:master Jan 7, 2020
@La0 La0 deleted the fix-in-patch branch January 7, 2020 11:47
@La0 La0 changed the title Fix bugs in load_in_patch script on Heroku Process diffs only once in load_in_patch Jan 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants