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

Reference task to fetch latest version by default #5895

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

featherchen
Copy link

@featherchen featherchen commented Oct 23, 2024

Tracking issue

Related to #5825

Why are the changes needed?

When task version is not specified (i.e. ""), we should return the latest one when getTask is called

What changes were proposed in this pull request?

When the version is "", we fetch DB directly to get the latest task.
Also remove the UT that check whether version is empty, since this situation is allowed now.

How was this patch tested?

By unit test (testGetTask)

Setup process

Screenshots

Check all the applicable boxes

  • I updated the documentation accordingly.
  • All new and existing tests passed.
  • All commits are signed-off.

Related PRs

Docs link

Copy link

welcome bot commented Oct 23, 2024

Thank you for opening this pull request! 🙌

These tips will help get your PR across the finish line:

  • Most of the repos have a PR template; if not, fill it out to the best of your knowledge.
  • Sign off your commits (Reference: DCO Guide).

@featherchen featherchen marked this pull request as draft October 23, 2024 09:44
Copy link

codecov bot commented Oct 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 33.03%. Comparing base (fef67b8) to head (fb6f97d).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5895      +/-   ##
==========================================
- Coverage   36.85%   33.03%   -3.83%     
==========================================
  Files        1310     1013     -297     
  Lines      131246   107363   -23883     
==========================================
- Hits        48377    35469   -12908     
+ Misses      78670    68757    -9913     
+ Partials     4199     3137    -1062     
Flag Coverage Δ
unittests-datacatalog 51.58% <ø> (ø)
unittests-flyteadmin ?
unittests-flytecopilot 11.73% <ø> (ø)
unittests-flytectl 62.39% <ø> (ø)
unittests-flyteidl 6.92% <ø> (ø)
unittests-flyteplugins 53.84% <ø> (ø)
unittests-flytepropeller 42.90% <ø> (ø)
unittests-flytestdlib 55.42% <ø> (+0.05%) ⬆️

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

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

@eapolinario
Copy link
Contributor

TesGetTask is failing, can you take a look?

@featherchen
Copy link
Author

@eapolinario TestGetTask should work now, thanks @troychiu for the fix.

@featherchen featherchen force-pushed the issue-5825 branch 6 times, most recently from 35f91cc to 7ff7fdc Compare November 8, 2024 05:18
@featherchen featherchen marked this pull request as ready for review November 8, 2024 05:30
@featherchen featherchen changed the title [WIP] Reference task to fetch latest version by default Reference task to fetch latest version by default Nov 8, 2024
@featherchen
Copy link
Author

@troychiu Can you help me review this PR? Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

3 participants