Fix flytectl returning the oldest workflow when using --latest flag #5716
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5716 +/- ##
=======================================
Coverage 36.16% 36.17%
=======================================
Files 1303 1303
Lines 109661 109676 +15
=======================================
+ Hits 39664 39673 +9
- Misses 65852 65858 +6
Partials 4145 4145
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Rafael Raposo <[email protected]>
Signed-off-by: Rafael Raposo <[email protected]>
RRap0so
force-pushed
the
rafaelraposo/fix-flytectl-latest
branch
from
d282732 to
53fe925
Compare
Signed-off-by: Rafael Raposo <[email protected]>
Signed-off-by: Rafael Raposo <[email protected]>
| filter := filters.Filters{ | ||
| SortBy: "created_at", | ||
| Limit: 1, | ||
| Asc: false, |
There was a problem hiding this comment.
I'm not entirely sure if I need to specify Asc but better safe than sorry.
There was a problem hiding this comment.
better be explicit yeah
RRap0so
changed the title
| @@ -55,7 +55,7 @@ type AdminFetcherExtInterface interface { | |||
| FetchAllVerOfWorkflow(ctx context.Context, name, project, domain string, filter filters.Filters) ([]*admin.Workflow, error) | |||
|
|
|||
| // FetchWorkflowLatestVersion fetches latest version of workflow in a project, domain | |||
| FetchWorkflowLatestVersion(ctx context.Context, name, project, domain string, filter filters.Filters) (*admin.Workflow, error) | |||
| FetchWorkflowLatestVersion(ctx context.Context, name, project, domain string) (*admin.Workflow, error) | |||
There was a problem hiding this comment.
Should we keep the filter here? What if I want to fetch the latest workflow with a specific tag?
There was a problem hiding this comment.
i think better to remove it actually, it's clearer in what it's doing, and merging filters behind the scenes seems weird to me.
There was a problem hiding this comment.
@RRap0so thanks for closing out a stale issue! you tested this locally against some running backend right? good to go if so.
| filter := filters.Filters{ | ||
| SortBy: "created_at", | ||
| Limit: 1, | ||
| Asc: false, |
There was a problem hiding this comment.
better be explicit yeah
| @@ -55,7 +55,7 @@ type AdminFetcherExtInterface interface { | |||
| FetchAllVerOfWorkflow(ctx context.Context, name, project, domain string, filter filters.Filters) ([]*admin.Workflow, error) | |||
|
|
|||
| // FetchWorkflowLatestVersion fetches latest version of workflow in a project, domain | |||
| FetchWorkflowLatestVersion(ctx context.Context, name, project, domain string, filter filters.Filters) (*admin.Workflow, error) | |||
| FetchWorkflowLatestVersion(ctx context.Context, name, project, domain string) (*admin.Workflow, error) | |||
There was a problem hiding this comment.
i think better to remove it actually, it's clearer in what it's doing, and merging filters behind the scenes seems weird to me.
|
@wild-endeavor I've tested it all. Going to go ahead and merge :) |
…lyteorg#5716) * Fix flytectl returning the oldest workflow when using --latest flag (flyteorg#5716) Signed-off-by: Rafael Raposo <[email protected]> Signed-off-by: Bugra Gedik <[email protected]>
Tracking issue
Closes #3972
Why are the changes needed?
Passing the --latest flag in when getting Workflows are not returning the latest but the opposite.
What changes were proposed in this pull request?
We set a default filter for the get
FetchWorkflowLatestVersionand alsoFetchLaunchPlanLatestVersion.How was this patch tested?
Setup process
Screenshots
Check all the applicable boxes
Related PRs
Docs link