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 <[email protected]>

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 &copy
 }

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),