Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add new_failure field to TextLogError and update when we have a… #8254

Merged
merged 1 commit into from
Oct 23, 2024

Conversation

jmaher
Copy link
Collaborator

@jmaher jmaher commented Oct 17, 2024

… new failure.

need to do:

  • database migration
  • unittests
  • check for other areas of code that might have interactions with this
  • consider perf impact

@jmaher jmaher self-assigned this Oct 17, 2024
@jmaher jmaher force-pushed the newfailure branch 5 times, most recently from 46547a8 to 9bae67c Compare October 21, 2024 14:17
@codecov-commenter
Copy link

codecov-commenter commented Oct 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.18%. Comparing base (d3b59c6) to head (375d58f).
Report is 8 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8254      +/-   ##
==========================================
+ Coverage   77.16%   77.18%   +0.01%     
==========================================
  Files         550      551       +1     
  Lines       27104    27123      +19     
  Branches     3361     3361              
==========================================
+ Hits        20915    20935      +20     
+ Misses       6028     6027       -1     
  Partials      161      161              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jmaher jmaher force-pushed the newfailure branch 2 times, most recently from 4fa2180 to 5d075df Compare October 21, 2024 23:12
@@ -30,6 +30,9 @@ def store_text_log_summary_artifact(job, text_log_summary_artifact):
ignore_conflicts=True,
)

# Bulk create doesn't return .id field, so query to get them.
log_errors = TextLogError.objects.filter(job=job)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

note, this could be a perf impact, we could reconsider this specific approach, maybe only do this if we have a need to update, or convert bulk_create -> save() in a loop

for tle in log_errors:
if tle.line_number == suggestion["line_number"]:
tle.new_failure = True
tle.save(update_fields=["new_failure"])
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

using update_fields is a perf win, so I get a little bit of a win here for the existing code :)

@jmaher jmaher changed the title WIP - add new_failure field to TextLogError and update when we have a… add new_failure field to TextLogError and update when we have a… Oct 22, 2024
tests/etl/test_load_artifacts.py Outdated Show resolved Hide resolved
@jmaher jmaher merged commit 3fd8b1b into master Oct 23, 2024
5 checks passed
@jmaher jmaher deleted the newfailure branch October 23, 2024 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants