From 2e49f752fb1497729274bf72c5b2c1a699be4731 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Murat=20Can=20=C3=9Cste?= Date: Wed, 1 Nov 2023 02:07:45 +0100 Subject: [PATCH] Fix project status change on update for archived projects (#438) Before, updating some field (i.e. `Description`) of an `Archived` project causes the project state to change to `Active`. This is caused by a bug in the `updateProjectsFunc` function used for the `update` command. During project updates, `edited`, a default project created based on the given configs/flags, comes with `admin.Project_ACTIVE` state by default if both `activate` and `archive` flags have not been set. Then a `copy` of the target project will be updated based on `edited` and then used for showing the diff. In the current implementation, the state of the `copy` is set to be the same as `edited` projects state, which is `Project_ACTIVE` if no flags are set. Also, after showing the diff, the `edited` is used for updating the project. Since it comes with `Project_ACTIVE` state by default, the target project will be updated to have `Project_ACTIVE` if both `activate` and `archive` flags are unset, regardless of the projects previous state. On this PR, we make sure we set the correct state to both `copy` and `edited` by checking if both `activate` and `archive` flags are set/unset. If one of the flags are set, we update target project state (`copy`) based on the `edited` state since it will have the desired state. If both flags are unset, we set the `edited` state to be the same as target (`copy`) state. This way, updating `archived` projects will not change their state. Signed-off-by: mcanueste --- flytectl/cmd/update/project.go | 34 +++++++++++++++++++++++++---- flytectl/cmd/update/project_test.go | 29 +++++++++++++++++++----- 2 files changed, 54 insertions(+), 9 deletions(-) diff --git a/flytectl/cmd/update/project.go b/flytectl/cmd/update/project.go index b9c2f17e90..215f2393d8 100644 --- a/flytectl/cmd/update/project.go +++ b/flytectl/cmd/update/project.go @@ -6,6 +6,7 @@ import ( "os" "github.com/flyteorg/flyte/flyteidl/gen/pb-go/flyteidl/admin" + "github.com/flyteorg/flytectl/clierrors" "github.com/flyteorg/flytectl/cmd/config" "github.com/flyteorg/flytectl/cmd/config/subcommand/project" @@ -85,7 +86,9 @@ Usage ) func updateProjectsFunc(ctx context.Context, args []string, cmdCtx cmdCore.CommandContext) error { - edits, err := project.DefaultProjectConfig.GetProjectSpec(config.GetConfig()) + projectConfig := project.DefaultProjectConfig + + edits, err := projectConfig.GetProjectSpec(config.GetConfig()) if err != nil { return err } @@ -103,7 +106,7 @@ func updateProjectsFunc(ctx context.Context, args []string, cmdCtx cmdCore.Comma // have a complete set of project's fields - it will only contain fields that // the update command allows updating. (For example, it won't have Domains field // initialized.) - currentProjectWithEdits := copyProjectWithEdits(currentProject, edits) + currentProjectWithEdits := copyProjectWithEdits(currentProject, edits, projectConfig) patch, err := DiffAsYaml(diffPathBefore, diffPathAfter, currentProject, currentProjectWithEdits) if err != nil { panic(err) @@ -136,10 +139,9 @@ func updateProjectsFunc(ctx context.Context, args []string, cmdCtx cmdCore.Comma // Makes a shallow copy of target and applies certain properties from edited to it. // The properties applied are only the ones supported by update command: state, name, // description, labels, etc. -func copyProjectWithEdits(target *admin.Project, edited *admin.Project) *admin.Project { +func copyProjectWithEdits(target *admin.Project, edited *admin.Project, projectConfig *project.ConfigProject) *admin.Project { copy := *target - copy.State = edited.State if edited.Name != "" { copy.Name = edited.Name } @@ -150,5 +152,29 @@ func copyProjectWithEdits(target *admin.Project, edited *admin.Project) *admin.P copy.Labels = edited.Labels } + // `edited` comes with `admin.Project_ACTIVE` state by default + // if both `activate` and `archive` flags have not been set. + // + // This will overwrite state of `copy` if we directly set it + // without checking for flags, which will show up on the diff. + // + // Also, after showing the diff, the `edited` is used for updating + // the project, which comes with `Project_ACTIVE` by default + // unless overwritten. Therefore, on the `else` block, + // we overwrite the `edited` with the state of `copy` + // if both `archive` and `activate` flags are unset. + // + // This is a bit hacky IMO. Proper solution would be to + // refactor `project.ConfigProject` and this file in order to + // separate the logic of setting `ConfigProject` struct fields + // from creation of a 'default' project based on those flags. + // Having a proper order of precedence between global config, + // YAML file input, and the flags for `ConfigProject` would also + // be good. + if projectConfig.Archive || projectConfig.Activate { + copy.State = edited.State + } else { + edited.State = copy.State + } return © } diff --git a/flytectl/cmd/update/project_test.go b/flytectl/cmd/update/project_test.go index 2f3fd11aed..1ef2c7b267 100644 --- a/flytectl/cmd/update/project_test.go +++ b/flytectl/cmd/update/project_test.go @@ -5,14 +5,13 @@ import ( "testing" "github.com/flyteorg/flyte/flyteidl/gen/pb-go/flyteidl/admin" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + + "github.com/flyteorg/flytectl/cmd/config" "github.com/flyteorg/flytectl/cmd/config/subcommand/project" "github.com/flyteorg/flytectl/cmd/testutils" "github.com/flyteorg/flytectl/pkg/ext" - - "github.com/flyteorg/flytectl/cmd/config" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/mock" ) func TestProjectCanBeActivated(t *testing.T) { @@ -184,6 +183,26 @@ func TestProjectUpdateRequiresProjectId(t *testing.T) { }) } +func TestProjectUpdateDoesNotActivateArchivedProject(t *testing.T) { + testProjectUpdate( + /* setup */ func(s *testutils.TestStruct, config *project.ConfigProject, project *admin.Project) { + project.State = admin.Project_ARCHIVED + config.Activate = false + config.Archive = false + config.Description = testutils.RandomName(12) + config.Force = true + }, + /* assert */ func(s *testutils.TestStruct, err error) { + assert.Nil(t, err) + s.MockAdminClient.AssertCalled( + t, "UpdateProject", s.Ctx, + mock.MatchedBy( + func(r *admin.Project) bool { + return r.State == admin.Project_ARCHIVED + })) + }) +} + func testProjectUpdate( setup func(s *testutils.TestStruct, config *project.ConfigProject, project *admin.Project), asserter func(s *testutils.TestStruct, err error),