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

Tiny url improvements #565

Merged
merged 10 commits into from
May 23, 2023
Merged

Tiny url improvements #565

merged 10 commits into from
May 23, 2023

Conversation

wild-endeavor
Copy link
Contributor

@wild-endeavor wild-endeavor commented May 17, 2023

TL;DR

This adds the ability to return an individual input or output in the flyte url, e.g.

flyte://v1/flytesnacks/development/ad5cb55f675g2s78swgp/end-node/i/o0

There's currently no mechanism by which these URLs can be returned to the frontend however. This can be worked on separately.

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

  • Refactored out the parsing logic to return a union instead. go doesn't have unions so returning a struct.
  • Split out the GetData function into one for node level and one for task execution level.
  • Added a second regex object to try to match against.

Tracking Issue

NA

Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Yee Hing Tong <[email protected]>
@codecov
Copy link

codecov bot commented May 17, 2023

Codecov Report

Merging #565 (de999e5) into master (7915ae8) will increase coverage by 1.62%.
The diff coverage is 75.00%.

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

@@            Coverage Diff             @@
##           master     #565      +/-   ##
==========================================
+ Coverage   58.51%   60.13%   +1.62%     
==========================================
  Files         168      168              
  Lines       16174    13285    -2889     
==========================================
- Hits         9464     7989    -1475     
+ Misses       5871     4454    -1417     
- Partials      839      842       +3     
Flag Coverage Δ
unittests ?

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

Impacted Files Coverage Δ
dataproxy/service.go 60.85% <67.64%> (+5.94%) ⬆️
pkg/common/flyte_url.go 94.28% <90.62%> (-0.96%) ⬇️

... and 153 files with indirect coverage changes

@wild-endeavor wild-endeavor marked this pull request as ready for review May 17, 2023 22:59
Signed-off-by: Yee Hing Tong <[email protected]>
eapolinario
eapolinario previously approved these changes May 18, 2023
Copy link
Contributor

@eapolinario eapolinario left a comment

Choose a reason for hiding this comment

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

one minor comment, otherwise, LGTM.

pkg/common/flyte_url_test.go Outdated Show resolved Hide resolved
Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: wild-endeavor <[email protected]>
Signed-off-by: Yee Hing Tong <[email protected]>
pingsutw
pingsutw previously approved these changes May 22, 2023
eapolinario
eapolinario previously approved these changes May 22, 2023
Copy link
Contributor

@eapolinario eapolinario left a comment

Choose a reason for hiding this comment

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

LGTM

return nil, errors.NewFlyteAdminErrorf(codes.InvalidArgument, "failed to validate resolve artifact request. Error: %v", err)
}

executions, err := common.ParseFlyteURLToExecution(req.GetFlyteUrl())
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: why is this in the plural?

Signed-off-by: Yee Hing Tong <[email protected]>
@wild-endeavor wild-endeavor dismissed stale reviews from eapolinario and pingsutw via 6ea405c May 23, 2023 01:02
@wild-endeavor wild-endeavor merged commit 15d5c7b into master May 23, 2023
@wild-endeavor wild-endeavor deleted the tiny-url-improvements branch May 23, 2023 17:52
eapolinario pushed a commit that referenced this pull request Sep 6, 2023
Signed-off-by: Yee Hing Tong <[email protected]>
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.

3 participants