From f30613bca7a6dbcd9324d5f4e24999977c47bfcf Mon Sep 17 00:00:00 2001 From: Rafael Raposo <100569684+RRap0so@users.noreply.github.com> Date: Mon, 9 Sep 2024 11:30:04 +0200 Subject: [PATCH] Fix flytectl returning the oldest workflow when using --latest flag (#5716) * Fix flytectl returning the oldest workflow when using --latest flag (#5716) Signed-off-by: Rafael Raposo Signed-off-by: Bugra Gedik --- flytectl/cmd/get/launch_plan.go | 2 +- flytectl/cmd/get/launch_plan_test.go | 5 ++- flytectl/cmd/get/workflow.go | 2 +- flytectl/cmd/get/workflow_test.go | 3 +- flytectl/pkg/ext/fetcher.go | 4 +-- flytectl/pkg/ext/launch_plan_fetcher.go | 9 +++-- flytectl/pkg/ext/launch_plan_fetcher_test.go | 4 +-- .../ext/mocks/admin_fetcher_ext_interface.go | 36 +++++++++---------- flytectl/pkg/ext/workflow_fetcher.go | 7 +++- flytectl/pkg/ext/workflow_fetcher_test.go | 4 +-- 10 files changed, 42 insertions(+), 34 deletions(-) diff --git a/flytectl/cmd/get/launch_plan.go b/flytectl/cmd/get/launch_plan.go index 2f32515339..79cede32c5 100644 --- a/flytectl/cmd/get/launch_plan.go +++ b/flytectl/cmd/get/launch_plan.go @@ -219,7 +219,7 @@ func FetchLPForName(ctx context.Context, fetcher ext.AdminFetcherExtInterface, n var lp *admin.LaunchPlan var err error if launchplan.DefaultConfig.Latest { - if lp, err = fetcher.FetchLPLatestVersion(ctx, name, project, domain, launchplan.DefaultConfig.Filter); err != nil { + if lp, err = fetcher.FetchLPLatestVersion(ctx, name, project, domain); err != nil { return nil, err } launchPlans = append(launchPlans, lp) diff --git a/flytectl/cmd/get/launch_plan_test.go b/flytectl/cmd/get/launch_plan_test.go index c197ec111e..87cc091535 100644 --- a/flytectl/cmd/get/launch_plan_test.go +++ b/flytectl/cmd/get/launch_plan_test.go @@ -293,11 +293,10 @@ func TestGetLaunchPlanFuncLatest(t *testing.T) { defer s.TearDown() getLaunchPlanSetup() launchplan.DefaultConfig.Latest = true - launchplan.DefaultConfig.Filter = filters.Filters{} - s.FetcherExt.OnFetchLPLatestVersionMatch(mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(launchPlan2, nil) + s.FetcherExt.OnFetchLPLatestVersionMatch(mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(launchPlan2, nil) err := getLaunchPlanFunc(s.Ctx, argsLp, s.CmdCtx) assert.Nil(t, err) - s.FetcherExt.AssertCalled(t, "FetchLPLatestVersion", s.Ctx, "launchplan1", projectValue, domainValue, launchplan.DefaultConfig.Filter) + s.FetcherExt.AssertCalled(t, "FetchLPLatestVersion", s.Ctx, "launchplan1", projectValue, domainValue) s.TearDownAndVerify(t, `{"id": {"name": "launchplan1","version": "v2"},"spec": {"workflowId": {"name": "workflow2"},"defaultInputs": {"parameters": {"generic": {"var": {"type": {"simple": "STRUCT"},"description": "generic"},"default": {"scalar": {"generic": {"foo": "foo"}}}},"numbers": {"var": {"type": {"collectionType": {"simple": "INTEGER"}},"description": "short desc"}},"numbers_count": {"var": {"type": {"simple": "INTEGER"},"description": "long description will be truncated in table"}},"run_local_at_count": {"var": {"type": {"simple": "INTEGER"},"description": "run_local_at_count"},"default": {"scalar": {"primitive": {"integer": "10"}}}}}}},"closure": {"expectedInputs": {"parameters": {"generic": {"var": {"type": {"simple": "STRUCT"},"description": "generic"},"default": {"scalar": {"generic": {"foo": "foo"}}}},"numbers": {"var": {"type": {"collectionType": {"simple": "INTEGER"}},"description": "short desc"}},"numbers_count": {"var": {"type": {"simple": "INTEGER"},"description": "long description will be truncated in table"}},"run_local_at_count": {"var": {"type": {"simple": "INTEGER"},"description": "run_local_at_count"},"default": {"scalar": {"primitive": {"integer": "10"}}}}}},"createdAt": "1970-01-01T00:00:01Z"}}`) } diff --git a/flytectl/cmd/get/workflow.go b/flytectl/cmd/get/workflow.go index 4fa8f93928..624e8d2ba8 100644 --- a/flytectl/cmd/get/workflow.go +++ b/flytectl/cmd/get/workflow.go @@ -183,7 +183,7 @@ func FetchWorkflowForName(ctx context.Context, fetcher ext.AdminFetcherExtInterf domain string) (workflows []*admin.Workflow, isList bool, err error) { var workflow *admin.Workflow if workflowconfig.DefaultConfig.Latest { - if workflow, err = fetcher.FetchWorkflowLatestVersion(ctx, name, project, domain, workflowconfig.DefaultConfig.Filter); err != nil { + if workflow, err = fetcher.FetchWorkflowLatestVersion(ctx, name, project, domain); err != nil { return nil, false, err } workflows = append(workflows, workflow) diff --git a/flytectl/cmd/get/workflow_test.go b/flytectl/cmd/get/workflow_test.go index 118ad03521..3e01067750 100644 --- a/flytectl/cmd/get/workflow_test.go +++ b/flytectl/cmd/get/workflow_test.go @@ -174,9 +174,8 @@ func TestGetWorkflowFuncLatestWithTable(t *testing.T) { getWorkflowSetup() workflow.DefaultConfig.Latest = true - workflow.DefaultConfig.Filter = filters.Filters{} config.GetConfig().Output = printer.OutputFormatTABLE.String() - s.FetcherExt.OnFetchWorkflowLatestVersionMatch(s.Ctx, "workflow1", projectValue, domainValue, filters.Filters{}).Return(workflow1, nil) + s.FetcherExt.OnFetchWorkflowLatestVersionMatch(s.Ctx, "workflow1", projectValue, domainValue).Return(workflow1, nil) err := getWorkflowFunc(s.Ctx, argsWf, s.CmdCtx) assert.Nil(t, err) s.TearDownAndVerify(t, ` diff --git a/flytectl/pkg/ext/fetcher.go b/flytectl/pkg/ext/fetcher.go index 1047e8d870..f6ec66d208 100644 --- a/flytectl/pkg/ext/fetcher.go +++ b/flytectl/pkg/ext/fetcher.go @@ -34,7 +34,7 @@ type AdminFetcherExtInterface interface { FetchAllVerOfLP(ctx context.Context, lpName, project, domain string, filter filters.Filters) ([]*admin.LaunchPlan, error) // FetchLPLatestVersion fetches latest version of launch plan in a project, domain - FetchLPLatestVersion(ctx context.Context, name, project, domain string, filter filters.Filters) (*admin.LaunchPlan, error) + FetchLPLatestVersion(ctx context.Context, name, project, domain string) (*admin.LaunchPlan, error) // FetchLPVersion fetches particular version of launch plan in a project, domain FetchLPVersion(ctx context.Context, name, version, project, domain string) (*admin.LaunchPlan, error) @@ -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) // FetchWorkflowVersion fetches particular version of workflow in a project, domain FetchWorkflowVersion(ctx context.Context, name, version, project, domain string) (*admin.Workflow, error) diff --git a/flytectl/pkg/ext/launch_plan_fetcher.go b/flytectl/pkg/ext/launch_plan_fetcher.go index 3c4ae96585..5a8befc093 100644 --- a/flytectl/pkg/ext/launch_plan_fetcher.go +++ b/flytectl/pkg/ext/launch_plan_fetcher.go @@ -26,8 +26,13 @@ func (a *AdminFetcherExtClient) FetchAllVerOfLP(ctx context.Context, lpName, pro } // FetchLPLatestVersion fetches latest version for give launch plan name -func (a *AdminFetcherExtClient) FetchLPLatestVersion(ctx context.Context, name, project, domain string, filter filters.Filters) (*admin.LaunchPlan, error) { - // Fetch the latest version of the task. +func (a *AdminFetcherExtClient) FetchLPLatestVersion(ctx context.Context, name, project, domain string) (*admin.LaunchPlan, error) { + // Fetch the latest version of the LaunchPLan. + filter := filters.Filters{ + SortBy: "created_at", + Limit: 1, + Asc: false, + } lpVersions, err := a.FetchAllVerOfLP(ctx, name, project, domain, filter) if err != nil { return nil, err diff --git a/flytectl/pkg/ext/launch_plan_fetcher_test.go b/flytectl/pkg/ext/launch_plan_fetcher_test.go index 61b78efd24..7eed907ff5 100644 --- a/flytectl/pkg/ext/launch_plan_fetcher_test.go +++ b/flytectl/pkg/ext/launch_plan_fetcher_test.go @@ -156,7 +156,7 @@ func TestFetchAllVerOfLPEmptyResponse(t *testing.T) { func TestFetchLPLatestVersion(t *testing.T) { getLaunchPlanFetcherSetup() adminClient.OnListLaunchPlansMatch(mock.Anything, mock.Anything).Return(launchPlanListResponse, nil) - _, err := adminFetcherExt.FetchLPLatestVersion(ctx, "lpName", "project", "domain", lpFilters) + _, err := adminFetcherExt.FetchLPLatestVersion(ctx, "lpName", "project", "domain") assert.Nil(t, err) } @@ -164,6 +164,6 @@ func TestFetchLPLatestVersionError(t *testing.T) { launchPlanListResponse := &admin.LaunchPlanList{} getLaunchPlanFetcherSetup() adminClient.OnListLaunchPlansMatch(mock.Anything, mock.Anything).Return(launchPlanListResponse, nil) - _, err := adminFetcherExt.FetchLPLatestVersion(ctx, "lpName", "project", "domain", lpFilters) + _, err := adminFetcherExt.FetchLPLatestVersion(ctx, "lpName", "project", "domain") assert.Equal(t, fmt.Errorf("no launchplans retrieved for lpName"), err) } diff --git a/flytectl/pkg/ext/mocks/admin_fetcher_ext_interface.go b/flytectl/pkg/ext/mocks/admin_fetcher_ext_interface.go index 514ae2ff39..42d4b0517d 100644 --- a/flytectl/pkg/ext/mocks/admin_fetcher_ext_interface.go +++ b/flytectl/pkg/ext/mocks/admin_fetcher_ext_interface.go @@ -266,8 +266,8 @@ func (_m AdminFetcherExtInterface_FetchLPLatestVersion) Return(_a0 *admin.Launch return &AdminFetcherExtInterface_FetchLPLatestVersion{Call: _m.Call.Return(_a0, _a1)} } -func (_m *AdminFetcherExtInterface) OnFetchLPLatestVersion(ctx context.Context, name string, project string, domain string, filter filters.Filters) *AdminFetcherExtInterface_FetchLPLatestVersion { - c_call := _m.On("FetchLPLatestVersion", ctx, name, project, domain, filter) +func (_m *AdminFetcherExtInterface) OnFetchLPLatestVersion(ctx context.Context, name string, project string, domain string) *AdminFetcherExtInterface_FetchLPLatestVersion { + c_call := _m.On("FetchLPLatestVersion", ctx, name, project, domain) return &AdminFetcherExtInterface_FetchLPLatestVersion{Call: c_call} } @@ -276,13 +276,13 @@ func (_m *AdminFetcherExtInterface) OnFetchLPLatestVersionMatch(matchers ...inte return &AdminFetcherExtInterface_FetchLPLatestVersion{Call: c_call} } -// FetchLPLatestVersion provides a mock function with given fields: ctx, name, project, domain, filter -func (_m *AdminFetcherExtInterface) FetchLPLatestVersion(ctx context.Context, name string, project string, domain string, filter filters.Filters) (*admin.LaunchPlan, error) { - ret := _m.Called(ctx, name, project, domain, filter) +// FetchLPLatestVersion provides a mock function with given fields: ctx, name, project, domain +func (_m *AdminFetcherExtInterface) FetchLPLatestVersion(ctx context.Context, name string, project string, domain string) (*admin.LaunchPlan, error) { + ret := _m.Called(ctx, name, project, domain) var r0 *admin.LaunchPlan - if rf, ok := ret.Get(0).(func(context.Context, string, string, string, filters.Filters) *admin.LaunchPlan); ok { - r0 = rf(ctx, name, project, domain, filter) + if rf, ok := ret.Get(0).(func(context.Context, string, string, string) *admin.LaunchPlan); ok { + r0 = rf(ctx, name, project, domain) } else { if ret.Get(0) != nil { r0 = ret.Get(0).(*admin.LaunchPlan) @@ -290,8 +290,8 @@ func (_m *AdminFetcherExtInterface) FetchLPLatestVersion(ctx context.Context, na } var r1 error - if rf, ok := ret.Get(1).(func(context.Context, string, string, string, filters.Filters) error); ok { - r1 = rf(ctx, name, project, domain, filter) + if rf, ok := ret.Get(1).(func(context.Context, string, string, string) error); ok { + r1 = rf(ctx, name, project, domain) } else { r1 = ret.Error(1) } @@ -676,8 +676,8 @@ func (_m AdminFetcherExtInterface_FetchWorkflowLatestVersion) Return(_a0 *admin. return &AdminFetcherExtInterface_FetchWorkflowLatestVersion{Call: _m.Call.Return(_a0, _a1)} } -func (_m *AdminFetcherExtInterface) OnFetchWorkflowLatestVersion(ctx context.Context, name string, project string, domain string, filter filters.Filters) *AdminFetcherExtInterface_FetchWorkflowLatestVersion { - c_call := _m.On("FetchWorkflowLatestVersion", ctx, name, project, domain, filter) +func (_m *AdminFetcherExtInterface) OnFetchWorkflowLatestVersion(ctx context.Context, name string, project string, domain string) *AdminFetcherExtInterface_FetchWorkflowLatestVersion { + c_call := _m.On("FetchWorkflowLatestVersion", ctx, name, project, domain) return &AdminFetcherExtInterface_FetchWorkflowLatestVersion{Call: c_call} } @@ -686,13 +686,13 @@ func (_m *AdminFetcherExtInterface) OnFetchWorkflowLatestVersionMatch(matchers . return &AdminFetcherExtInterface_FetchWorkflowLatestVersion{Call: c_call} } -// FetchWorkflowLatestVersion provides a mock function with given fields: ctx, name, project, domain, filter -func (_m *AdminFetcherExtInterface) FetchWorkflowLatestVersion(ctx context.Context, name string, project string, domain string, filter filters.Filters) (*admin.Workflow, error) { - ret := _m.Called(ctx, name, project, domain, filter) +// FetchWorkflowLatestVersion provides a mock function with given fields: ctx, name, project, domain +func (_m *AdminFetcherExtInterface) FetchWorkflowLatestVersion(ctx context.Context, name string, project string, domain string) (*admin.Workflow, error) { + ret := _m.Called(ctx, name, project, domain) var r0 *admin.Workflow - if rf, ok := ret.Get(0).(func(context.Context, string, string, string, filters.Filters) *admin.Workflow); ok { - r0 = rf(ctx, name, project, domain, filter) + if rf, ok := ret.Get(0).(func(context.Context, string, string, string) *admin.Workflow); ok { + r0 = rf(ctx, name, project, domain) } else { if ret.Get(0) != nil { r0 = ret.Get(0).(*admin.Workflow) @@ -700,8 +700,8 @@ func (_m *AdminFetcherExtInterface) FetchWorkflowLatestVersion(ctx context.Conte } var r1 error - if rf, ok := ret.Get(1).(func(context.Context, string, string, string, filters.Filters) error); ok { - r1 = rf(ctx, name, project, domain, filter) + if rf, ok := ret.Get(1).(func(context.Context, string, string, string) error); ok { + r1 = rf(ctx, name, project, domain) } else { r1 = ret.Error(1) } diff --git a/flytectl/pkg/ext/workflow_fetcher.go b/flytectl/pkg/ext/workflow_fetcher.go index 7f3624f512..69032bb998 100644 --- a/flytectl/pkg/ext/workflow_fetcher.go +++ b/flytectl/pkg/ext/workflow_fetcher.go @@ -42,8 +42,13 @@ func (a *AdminFetcherExtClient) FetchAllWorkflows(ctx context.Context, project, } // FetchWorkflowLatestVersion fetches latest version for given workflow name -func (a *AdminFetcherExtClient) FetchWorkflowLatestVersion(ctx context.Context, name, project, domain string, filter filters.Filters) (*admin.Workflow, error) { +func (a *AdminFetcherExtClient) FetchWorkflowLatestVersion(ctx context.Context, name, project, domain string) (*admin.Workflow, error) { // Fetch the latest version of the workflow. + filter := filters.Filters{ + SortBy: "created_at", + Limit: 1, + Asc: false, + } wVersions, err := a.FetchAllVerOfWorkflow(ctx, name, project, domain, filter) if err != nil { return nil, err diff --git a/flytectl/pkg/ext/workflow_fetcher_test.go b/flytectl/pkg/ext/workflow_fetcher_test.go index 114d6afea4..c6044eb656 100644 --- a/flytectl/pkg/ext/workflow_fetcher_test.go +++ b/flytectl/pkg/ext/workflow_fetcher_test.go @@ -147,7 +147,7 @@ func TestFetchWorkflowLatestVersion(t *testing.T) { getWorkflowFetcherSetup() adminClient.OnGetWorkflowMatch(mock.Anything, mock.Anything).Return(workflowResponse, nil) adminClient.OnListWorkflowsMatch(mock.Anything, mock.Anything).Return(workflowListResponse, nil) - _, err := adminFetcherExt.FetchWorkflowLatestVersion(ctx, "workflowName", "project", "domain", workflowFilter) + _, err := adminFetcherExt.FetchWorkflowLatestVersion(ctx, "workflowName", "project", "domain") assert.Nil(t, err) } @@ -155,6 +155,6 @@ func TestFetchWorkflowLatestVersionError(t *testing.T) { workflowListResponse := &admin.WorkflowList{} getWorkflowFetcherSetup() adminClient.OnListWorkflowsMatch(mock.Anything, mock.Anything).Return(workflowListResponse, nil) - _, err := adminFetcherExt.FetchWorkflowLatestVersion(ctx, "workflowName", "project", "domain", workflowFilter) + _, err := adminFetcherExt.FetchWorkflowLatestVersion(ctx, "workflowName", "project", "domain") assert.Equal(t, fmt.Errorf("no workflow retrieved for workflowName"), err) }