Skip to content
This repository has been archived by the owner on May 31, 2024. It is now read-only.

Use proto file name to infer message type #436

Merged
merged 2 commits into from
Oct 23, 2023

Conversation

honnix
Copy link
Member

@honnix honnix commented Oct 20, 2023

TL;DR

Use proto file name to infer message type to have more precise unmarshalling.

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added (the existing unit tests cover this change well)
  • Code documentation added
  • Any pending items have an associated Issue

Complete description

flytekit generated proto file name has a pattern to precisely indicate the message type: .*_1.pb is task, .*_2.pb is workflow and .*_3.pb is launch plan. We can rely on this pattern to do more precise unmarshalling. Note that we still try other types if the unmarshalling based on the precisely matched type does not work.

This PR keeps backward compatibility so if the pattern is not seen, it goes through all possible types and tries one by one.

Tracking Issue

fixes flyteorg/flyte#3042

Follow-up issue

NA

}

var reordered []unMarshalFunc
for i, f := range unMarshalFuncs {
Copy link
Member Author

Choose a reason for hiding this comment

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

This loop basically bubbles up the matched function to the first place.

@codecov
Copy link

codecov bot commented Oct 20, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (cdfa222) 67.38% compared to head (2da1cbb) 68.12%.

❗ Current head 2da1cbb differs from pull request most recent head 2394e7b. Consider uploading reports for the commit 2394e7b to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #436      +/-   ##
==========================================
+ Coverage   67.38%   68.12%   +0.74%     
==========================================
  Files         148      148              
  Lines        6585     5494    -1091     
==========================================
- Hits         4437     3743     -694     
+ Misses       1858     1460     -398     
- Partials      290      291       +1     
Flag Coverage Δ
unittests ?

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

Files Coverage Δ
cmd/register/register_util.go 78.11% <93.33%> (+2.51%) ⬆️

... and 135 files with indirect coverage changes

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

@honnix honnix marked this pull request as ready for review October 20, 2023 07:09
@honnix honnix requested review from pmahindrakar-oss and EngHabu and removed request for EngHabu, wild-endeavor and kumare3 October 20, 2023 07:10
@@ -92,37 +92,72 @@ var projectColumns = []printer.Column{
{Header: "Additional Info", JSONPath: "$.Info"},
}

var fnameRegex = regexp.MustCompile(`^.*_(?P<index>[1-3])\.pb$`)
Copy link
Member

@pingsutw pingsutw Oct 20, 2023

Choose a reason for hiding this comment

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

nit: could we add some comments here

Copy link
Member Author

Choose a reason for hiding this comment

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

Added in a latter commit.

Signed-off-by: Hongxin Liang <[email protected]>
@honnix honnix requested a review from pingsutw October 23, 2023 10:58
@honnix
Copy link
Member Author

honnix commented Oct 23, 2023

Thank you for approving. Could you please help merge it when ready? Thanks.

@pingsutw pingsutw merged commit 1eda51e into flyteorg:master Oct 23, 2023
14 checks passed
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] Flyte protobuf registration should explicitly have a type information
2 participants