Skip to content
This repository has been archived by the owner on Oct 9, 2023. It is now read-only.

Fix utf-8 encoding issues with trimmed error messages #569

Merged
merged 9 commits into from
May 30, 2023

Conversation

katrogan
Copy link
Contributor

@katrogan katrogan commented May 30, 2023

TL;DR
Trimmed error messages returned in the list executions view are not always valid utf-8 strings. This changes ensures we trim error messages at valid utf-8 char boundaries

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Complete description

Verified on an internal flyte deployment

Tracking Issue

fixes flyteorg/flyte#3729

Follow-up issue

NA

Signed-off-by: Katrina Rogan <[email protected]>
@katrogan katrogan changed the title Not for review: https://github.com/flyteorg/flyte/issues/3729 Not for review: debug utf-8 encoding issues May 30, 2023
@codecov
Copy link

codecov bot commented May 30, 2023

Codecov Report

Merging #569 (fcc599b) into master (15d5c7b) will increase coverage by 1.54%.
The diff coverage is 100.00%.

❗ Current head fcc599b differs from pull request most recent head ebae8c8. Consider uploading reports for the commit ebae8c8 to get more accurate results

@@            Coverage Diff             @@
##           master     #569      +/-   ##
==========================================
+ Coverage   58.63%   60.17%   +1.54%     
==========================================
  Files         168      168              
  Lines       16251    13299    -2952     
==========================================
- Hits         9528     8003    -1525     
+ Misses       5881     4454    -1427     
  Partials      842      842              
Flag Coverage Δ
unittests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/manager/impl/node_execution_manager.go 73.22% <100.00%> (+3.15%) ⬆️
pkg/repositories/transformers/execution.go 83.53% <100.00%> (+2.80%) ⬆️
pkg/repositories/transformers/node_execution.go 75.60% <100.00%> (+3.05%) ⬆️

... and 152 files with indirect coverage changes

katrogan added 4 commits May 30, 2023 12:08
Signed-off-by: Katrina Rogan <[email protected]>
Signed-off-by: Katrina Rogan <[email protected]>
Signed-off-by: Katrina Rogan <[email protected]>
@katrogan katrogan changed the title Not for review: debug utf-8 encoding issues Fix utf-8 encoding issues with trimmed error messages May 30, 2023
@katrogan katrogan marked this pull request as ready for review May 30, 2023 19:34
katrogan added 2 commits May 30, 2023 12:34
Signed-off-by: Katrina Rogan <[email protected]>
Signed-off-by: Katrina Rogan <[email protected]>
wild-endeavor
wild-endeavor previously approved these changes May 30, 2023
hamersaw
hamersaw previously approved these changes May 30, 2023
Signed-off-by: Katrina Rogan <[email protected]>
@katrogan katrogan dismissed stale reviews from hamersaw and wild-endeavor via f8144c7 May 30, 2023 19:50
wild-endeavor
wild-endeavor previously approved these changes May 30, 2023
Signed-off-by: Katrina Rogan <[email protected]>
@katrogan katrogan merged commit 3379031 into master May 30, 2023
@katrogan katrogan deleted the utf-8-breakage branch May 30, 2023 20:26
Copy link
Contributor

@EngHabu EngHabu left a comment

Choose a reason for hiding this comment

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

Do we need to cherry pick this to a previous release? Or release a patch for flyte 1.6?

@eapolinario
Copy link
Contributor

For sure. We're going to have a patch release tomorrow morning.

@eapolinario
Copy link
Contributor

Flyte 1.6.2 is out and contains this fix.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Ensure execution proto message values are utf-8 compliant
5 participants