From a9a45b3576852a640a53438491a31235236d0336 Mon Sep 17 00:00:00 2001 From: Soundarya Alagesan Date: Tue, 26 Dec 2023 20:19:20 +0530 Subject: [PATCH] Feature: update launchplan --archive to --deactivate (#449) * Rename --archive to --deactivate in update launchplan Signed-off-by: asoundarya96 * Rename --archive to --deactivate in update launchplan Signed-off-by: asoundarya96 * Keep --archive as deprecated flag Signed-off-by: asoundarya96 * make generate Signed-off-by: Eduardo Apolinario --------- Signed-off-by: asoundarya96 Signed-off-by: Eduardo Apolinario Co-authored-by: Eduardo Apolinario --- flytectl/clierrors/errors.go | 3 ++- .../subcommand/launchplan/updateconfig.go | 11 +++++---- .../launchplan/updateconfig_flags.go | 3 ++- .../launchplan/updateconfig_flags_test.go | 22 +++++++++++++---- flytectl/cmd/update/launch_plan.go | 23 ++++++++++++++---- flytectl/cmd/update/launch_plan_test.go | 24 ++++++++++++++++--- .../source/gen/flytectl_update_launchplan.rst | 9 +++---- 7 files changed, 72 insertions(+), 23 deletions(-) mode change 100755 => 100644 flytectl/cmd/config/subcommand/launchplan/updateconfig_flags.go diff --git a/flytectl/clierrors/errors.go b/flytectl/clierrors/errors.go index 05ab96cb00..48fecd3f2f 100644 --- a/flytectl/clierrors/errors.go +++ b/flytectl/clierrors/errors.go @@ -1,7 +1,8 @@ package clierrors var ( - ErrInvalidStateUpdate = "invalid state passed. Specify either activate or archive\n" + ErrInvalidStateUpdate = "invalid state passed. Specify either activate or archive\n" + ErrInvalidBothStateUpdate = "invalid state passed. Specify either activate or deactivate\n" ErrProjectNotPassed = "project id wasn't passed\n" // #nosec ErrProjectIDBothPassed = "both project and id are passed\n" diff --git a/flytectl/cmd/config/subcommand/launchplan/updateconfig.go b/flytectl/cmd/config/subcommand/launchplan/updateconfig.go index 36e353c2e1..5d3b113dac 100644 --- a/flytectl/cmd/config/subcommand/launchplan/updateconfig.go +++ b/flytectl/cmd/config/subcommand/launchplan/updateconfig.go @@ -7,9 +7,10 @@ var ( // Config type UpdateConfig struct { - Archive bool `json:"archive" pflag:",disable the launch plan schedule (if it has an active schedule associated with it)."` - Activate bool `json:"activate" pflag:",activate launchplan."` - DryRun bool `json:"dryRun" pflag:",execute command without making any modifications."` - Force bool `json:"force" pflag:",do not ask for an acknowledgement during updates."` - Version string `json:"version" pflag:",version of the launchplan to be fetched."` + Activate bool `json:"activate" pflag:",activate launchplan."` + Archive bool `json:"archive" pflag:",(Deprecated) disable the launch plan schedule (if it has an active schedule associated with it)."` + Deactivate bool `json:"deactivate" pflag:",disable the launch plan schedule (if it has an active schedule associated with it)."` + DryRun bool `json:"dryRun" pflag:",execute command without making any modifications."` + Force bool `json:"force" pflag:",do not ask for an acknowledgement during updates."` + Version string `json:"version" pflag:",version of the launchplan to be fetched."` } diff --git a/flytectl/cmd/config/subcommand/launchplan/updateconfig_flags.go b/flytectl/cmd/config/subcommand/launchplan/updateconfig_flags.go old mode 100755 new mode 100644 index 4a9cad23ba..b71224e72b --- a/flytectl/cmd/config/subcommand/launchplan/updateconfig_flags.go +++ b/flytectl/cmd/config/subcommand/launchplan/updateconfig_flags.go @@ -50,8 +50,9 @@ func (UpdateConfig) mustMarshalJSON(v json.Marshaler) string { // flags is json-name.json-sub-name... etc. func (cfg UpdateConfig) GetPFlagSet(prefix string) *pflag.FlagSet { cmdFlags := pflag.NewFlagSet("UpdateConfig", pflag.ExitOnError) - cmdFlags.BoolVar(&UConfig.Archive, fmt.Sprintf("%v%v", prefix, "archive"), UConfig.Archive, "disable the launch plan schedule (if it has an active schedule associated with it).") cmdFlags.BoolVar(&UConfig.Activate, fmt.Sprintf("%v%v", prefix, "activate"), UConfig.Activate, "activate launchplan.") + cmdFlags.BoolVar(&UConfig.Archive, fmt.Sprintf("%v%v", prefix, "archive"), UConfig.Archive, "(Deprecated) disable the launch plan schedule (if it has an active schedule associated with it).") + cmdFlags.BoolVar(&UConfig.Deactivate, fmt.Sprintf("%v%v", prefix, "deactivate"), UConfig.Deactivate, "disable the launch plan schedule (if it has an active schedule associated with it).") cmdFlags.BoolVar(&UConfig.DryRun, fmt.Sprintf("%v%v", prefix, "dryRun"), UConfig.DryRun, "execute command without making any modifications.") cmdFlags.BoolVar(&UConfig.Force, fmt.Sprintf("%v%v", prefix, "force"), UConfig.Force, "do not ask for an acknowledgement during updates.") cmdFlags.StringVar(&UConfig.Version, fmt.Sprintf("%v%v", prefix, "version"), UConfig.Version, "version of the launchplan to be fetched.") diff --git a/flytectl/cmd/config/subcommand/launchplan/updateconfig_flags_test.go b/flytectl/cmd/config/subcommand/launchplan/updateconfig_flags_test.go index e9acca7bbe..fc58e7ac8f 100755 --- a/flytectl/cmd/config/subcommand/launchplan/updateconfig_flags_test.go +++ b/flytectl/cmd/config/subcommand/launchplan/updateconfig_flags_test.go @@ -99,6 +99,20 @@ func TestUpdateConfig_SetFlags(t *testing.T) { cmdFlags := actual.GetPFlagSet("") assert.True(t, cmdFlags.HasFlags()) + t.Run("Test_activate", func(t *testing.T) { + + t.Run("Override", func(t *testing.T) { + testValue := "1" + + cmdFlags.Set("activate", testValue) + if vBool, err := cmdFlags.GetBool("activate"); err == nil { + testDecodeJson_UpdateConfig(t, fmt.Sprintf("%v", vBool), &actual.Activate) + + } else { + assert.FailNow(t, err.Error()) + } + }) + }) t.Run("Test_archive", func(t *testing.T) { t.Run("Override", func(t *testing.T) { @@ -113,14 +127,14 @@ func TestUpdateConfig_SetFlags(t *testing.T) { } }) }) - t.Run("Test_activate", func(t *testing.T) { + t.Run("Test_deactivate", func(t *testing.T) { t.Run("Override", func(t *testing.T) { testValue := "1" - cmdFlags.Set("activate", testValue) - if vBool, err := cmdFlags.GetBool("activate"); err == nil { - testDecodeJson_UpdateConfig(t, fmt.Sprintf("%v", vBool), &actual.Activate) + cmdFlags.Set("deactivate", testValue) + if vBool, err := cmdFlags.GetBool("deactivate"); err == nil { + testDecodeJson_UpdateConfig(t, fmt.Sprintf("%v", vBool), &actual.Deactivate) } else { assert.FailNow(t, err.Error()) diff --git a/flytectl/cmd/update/launch_plan.go b/flytectl/cmd/update/launch_plan.go index 28b7c6270b..b20ad48040 100644 --- a/flytectl/cmd/update/launch_plan.go +++ b/flytectl/cmd/update/launch_plan.go @@ -7,6 +7,7 @@ import ( "github.com/flyteorg/flyte/flyteidl/gen/pb-go/flyteidl/admin" "github.com/flyteorg/flyte/flyteidl/gen/pb-go/flyteidl/core" + "github.com/flyteorg/flyte/flytestdlib/logger" "github.com/flyteorg/flytectl/clierrors" "github.com/flyteorg/flytectl/cmd/config" "github.com/flyteorg/flytectl/cmd/config/subcommand/launchplan" @@ -22,10 +23,10 @@ Activates a ` + "`launch plan `__" + ` a launch plan which deschedules any scheduled job associated with it: +Deactivates a ` + "`launch plan `__" + ` which deschedules any scheduled job associated with it: :: - flytectl update launchplan -p flytesnacks -d development core.control_flow.merge_sort.merge_sort --version v1 --archive + flytectl update launchplan -p flytesnacks -d development core.control_flow.merge_sort.merge_sort --version v1 --deactivate Usage ` @@ -45,14 +46,22 @@ func updateLPFunc(ctx context.Context, args []string, cmdCtx cmdCore.CommandCont activate := launchplan.UConfig.Activate archive := launchplan.UConfig.Archive - if activate == archive && archive { - return fmt.Errorf(clierrors.ErrInvalidStateUpdate) + + var deactivate bool + if archive { + deprecatedCommandWarning(ctx, "archive", "deactivate") + deactivate = true + } else { + deactivate = launchplan.UConfig.Deactivate + } + if activate == deactivate && deactivate { + return fmt.Errorf(clierrors.ErrInvalidBothStateUpdate) } var newState admin.LaunchPlanState if activate { newState = admin.LaunchPlanState_ACTIVE - } else if archive { + } else if deactivate { newState = admin.LaunchPlanState_INACTIVE } @@ -106,3 +115,7 @@ func updateLPFunc(ctx context.Context, args []string, cmdCtx cmdCore.CommandCont return nil } + +func deprecatedCommandWarning(ctx context.Context, oldCommand string, newCommand string) { + logger.Warningf(ctx, "--%v is deprecated, Please use --%v", oldCommand, newCommand) +} diff --git a/flytectl/cmd/update/launch_plan_test.go b/flytectl/cmd/update/launch_plan_test.go index f9c3d7dc8a..4bc92ef095 100644 --- a/flytectl/cmd/update/launch_plan_test.go +++ b/flytectl/cmd/update/launch_plan_test.go @@ -53,14 +53,32 @@ func TestLaunchPlanCanBeArchived(t *testing.T) { }) } -func TestLaunchPlanCannotBeActivatedAndArchivedAtTheSameTime(t *testing.T) { +func TestLaunchPlanCanBeDeactivated(t *testing.T) { + testLaunchPlanUpdate( + /* setup */ func(s *testutils.TestStruct, config *launchplan.UpdateConfig, launchplan *admin.LaunchPlan) { + launchplan.Closure.State = admin.LaunchPlanState_ACTIVE + config.Deactivate = true + config.Force = true + }, + /* assert */ func(s *testutils.TestStruct, err error) { + assert.Nil(t, err) + s.MockAdminClient.AssertCalled( + t, "UpdateLaunchPlan", s.Ctx, + mock.MatchedBy( + func(r *admin.LaunchPlanUpdateRequest) bool { + return r.State == admin.LaunchPlanState_INACTIVE + })) + }) +} + +func TestLaunchPlanCannotBeActivatedAndDeactivatedAtTheSameTime(t *testing.T) { testLaunchPlanUpdate( /* setup */ func(s *testutils.TestStruct, config *launchplan.UpdateConfig, launchplan *admin.LaunchPlan) { config.Activate = true - config.Archive = true + config.Deactivate = true }, /* assert */ func(s *testutils.TestStruct, err error) { - assert.ErrorContains(t, err, "Specify either activate or archive") + assert.ErrorContains(t, err, "Specify either activate or deactivate") s.MockAdminClient.AssertNotCalled(t, "UpdateLaunchPlan", mock.Anything, mock.Anything) }) } diff --git a/flytectl/docs/source/gen/flytectl_update_launchplan.rst b/flytectl/docs/source/gen/flytectl_update_launchplan.rst index 7fafd67129..b6de8566a5 100644 --- a/flytectl/docs/source/gen/flytectl_update_launchplan.rst +++ b/flytectl/docs/source/gen/flytectl_update_launchplan.rst @@ -15,10 +15,10 @@ Activates a `launch plan `__ a launch plan which deschedules any scheduled job associated with it: +Deactivates a `launch plan `__ which deschedules any scheduled job associated with it: :: - flytectl update launchplan -p flytesnacks -d development core.control_flow.merge_sort.merge_sort --version v1 --archive + flytectl update launchplan -p flytesnacks -d development core.control_flow.merge_sort.merge_sort --version v1 --deactivate Usage @@ -33,7 +33,7 @@ Options :: --activate activate launchplan. - --archive disable the launch plan schedule (if it has an active schedule associated with it). + --deactivate disable the launch plan schedule (if it has an active schedule associated with it). --dryRun execute command without making any modifications. --force do not ask for an acknowledgement during updates. -h, --help help for launchplan @@ -53,11 +53,12 @@ Options inherited from parent commands --admin.clientSecretEnvVar string Environment variable containing the client secret --admin.clientSecretLocation string File containing the client secret (default "/etc/secrets/client_secret") --admin.command strings Command for external authentication token generation - --admin.defaultServiceConfig string + --admin.defaultServiceConfig string --admin.deviceFlowConfig.pollInterval string amount of time the device flow would poll the token endpoint if auth server doesn't return a polling interval. Okta and google IDP do return an interval' (default "5s") --admin.deviceFlowConfig.refreshTime string grace period from the token expiry after which it would refresh the token. (default "5m0s") --admin.deviceFlowConfig.timeout string amount of time the device flow should complete or else it will be cancelled. (default "10m0s") --admin.endpoint string For admin types, specify where the uri of the service is located. + --admin.httpProxyURL string OPTIONAL: HTTP Proxy to be used for OAuth requests. --admin.insecure Use insecure connection. --admin.insecureSkipVerify InsecureSkipVerify controls whether a client verifies the server's certificate chain and host name. Caution : shouldn't be use for production usecases' --admin.maxBackoffDelay string Max delay for grpc backoff (default "8s")