Skip to content

Commit

Permalink
Refactored the fetcher interface to exclude cmdCtx (#61)
Browse files Browse the repository at this point in the history
* Refactored the fetcher interface to exclude cmdCtx and added builder

Signed-off-by: Prafulla Mahindrakar <[email protected]>

* Further refactoring and fixed tests

Signed-off-by: Prafulla Mahindrakar <[email protected]>

* Added more coverage and fixed linter issues

Signed-off-by: Prafulla Mahindrakar <[email protected]>

* Still more coverage

Signed-off-by: Prafulla Mahindrakar <[email protected]>

* Still more coverage

Signed-off-by: Prafulla Mahindrakar <[email protected]>

* lint issue

Signed-off-by: Prafulla Mahindrakar <[email protected]>

* lint issue

Signed-off-by: Prafulla Mahindrakar <[email protected]>

* morelint issue

Signed-off-by: Prafulla Mahindrakar <[email protected]>

* Renamed Fetcher to be AdminFetcherExt and initialized it cmdCtx

Signed-off-by: Prafulla Mahindrakar <[email protected]>
  • Loading branch information
pmahindrakar-oss authored May 7, 2021
1 parent f103e41 commit a0632f0
Show file tree
Hide file tree
Showing 27 changed files with 1,218 additions and 428 deletions.
1 change: 0 additions & 1 deletion flytectl/cmd/core/cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,6 @@ func generateCommandFunc(cmdEntry CommandEntry) func(cmd *cobra.Command, args []
if err != nil {
return err
}

return cmdEntry.CmdFunc(ctx, args, NewCommandContext(clientSet.AdminClient(), cmd.OutOrStdout()))
}
}
15 changes: 11 additions & 4 deletions flytectl/cmd/core/cmd_ctx.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,17 +3,20 @@ package cmdcore
import (
"io"

"github.com/flyteorg/flytectl/pkg/ext"
"github.com/flyteorg/flyteidl/gen/pb-go/flyteidl/service"
)

type CommandContext struct {
adminClient service.AdminServiceClient
in io.Reader
out io.Writer
adminClient service.AdminServiceClient
adminClientFetcherExt ext.AdminFetcherExtInterface
in io.Reader
out io.Writer
}

func NewCommandContext(adminClient service.AdminServiceClient, out io.Writer) CommandContext {
return CommandContext{adminClient: adminClient, out: out}
return CommandContext{adminClient: adminClient, out: out,
adminClientFetcherExt: &ext.AdminFetcherExtClient{AdminClient: adminClient}}
}

func (c CommandContext) AdminClient() service.AdminServiceClient {
Expand All @@ -27,3 +30,7 @@ func (c CommandContext) OutputPipe() io.Writer {
func (c CommandContext) InputPipe() io.Reader {
return c.in
}

func (c CommandContext) AdminFetcherExt() ext.AdminFetcherExtInterface {
return c.adminClientFetcherExt
}
3 changes: 2 additions & 1 deletion flytectl/cmd/create/create.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package create

import (
cmdcore "github.com/flyteorg/flytectl/cmd/core"

"github.com/spf13/cobra"
)

Expand All @@ -16,7 +17,7 @@ Example create.
`
)

// CreateCommand will return create command
// RemoteCreateCommand will return create flyte resource commands
func RemoteCreateCommand() *cobra.Command {
createCmd := &cobra.Command{
Use: "create",
Expand Down
2 changes: 1 addition & 1 deletion flytectl/cmd/create/execution_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ func TestCreateLaunchPlanExecutionFunc(t *testing.T) {
err = createExecutionCommand(ctx, args, cmdCtx)
assert.Nil(t, err)
mockClient.AssertCalled(t, "CreateExecution", ctx, mock.Anything)
tearDownAndVerify(t, `execution identifier project:"flytesnacks" domain:"development" name:"f652ea3596e7f4d80a0e"`)
tearDownAndVerify(t, `execution identifier project:"flytesnacks" domain:"development" name:"f652ea3596e7f4d80a0e" `)
}

func TestCreateRelaunchExecutionFunc(t *testing.T) {
Expand Down
26 changes: 18 additions & 8 deletions flytectl/cmd/create/execution_util.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,31 +14,37 @@ import (
"sigs.k8s.io/yaml"
)

func createExecutionRequestForWorkflow(ctx context.Context, workflowName string, project string, domain string, cmdCtx cmdCore.CommandContext) (*admin.ExecutionCreateRequest, error) {
func createExecutionRequestForWorkflow(ctx context.Context, workflowName, project, domain string,
cmdCtx cmdCore.CommandContext) (*admin.ExecutionCreateRequest, error) {
var lp *admin.LaunchPlan
var err error

// Fetch the launch plan
if lp, err = cmdGet.DefaultFetcher.FetchLPVersion(ctx, workflowName, executionConfig.Version, project, domain, cmdCtx); err != nil {
if lp, err = cmdCtx.AdminFetcherExt().FetchLPVersion(ctx, workflowName, executionConfig.Version, project, domain); err != nil {
return nil, err
}

// Create workflow params literal map
var paramLiterals map[string]*core.Literal
workflowParams := cmdGet.WorkflowParams(lp)

if paramLiterals, err = MakeLiteralForParams(executionConfig.Inputs, workflowParams); err != nil {
return nil, err
}
var inputs = &core.LiteralMap{
Literals: paramLiterals,
}

ID := lp.Id
return createExecutionRequest(ID, inputs, nil), nil
}

func createExecutionRequestForTask(ctx context.Context, taskName string, project string, domain string, cmdCtx cmdCore.CommandContext) (*admin.ExecutionCreateRequest, error) {
func createExecutionRequestForTask(ctx context.Context, taskName string, project string, domain string,
cmdCtx cmdCore.CommandContext) (*admin.ExecutionCreateRequest, error) {
var task *admin.Task
var err error
// Fetch the task
if task, err = cmdGet.FetchTaskVersion(ctx, taskName, executionConfig.Version, project, domain, cmdCtx); err != nil {
if task, err = cmdCtx.AdminFetcherExt().FetchTaskVersion(ctx, taskName, executionConfig.Version, project, domain); err != nil {
return nil, err
}
// Create task variables literal map
Expand Down Expand Up @@ -68,7 +74,8 @@ func createExecutionRequestForTask(ctx context.Context, taskName string, project
return createExecutionRequest(ID, inputs, authRole), nil
}

func relaunchExecution(ctx context.Context, executionName string, project string, domain string, cmdCtx cmdCore.CommandContext) error {
func relaunchExecution(ctx context.Context, executionName string, project string, domain string,
cmdCtx cmdCore.CommandContext) error {
relaunchedExec, err := cmdCtx.AdminClient().RelaunchExecution(ctx, &admin.ExecutionRelaunchRequest{
Id: &core.WorkflowExecutionIdentifier{
Name: executionName,
Expand All @@ -83,7 +90,8 @@ func relaunchExecution(ctx context.Context, executionName string, project string
return nil
}

func createExecutionRequest(ID *core.Identifier, inputs *core.LiteralMap, authRole *admin.AuthRole) *admin.ExecutionCreateRequest {
func createExecutionRequest(ID *core.Identifier, inputs *core.LiteralMap,
authRole *admin.AuthRole) *admin.ExecutionCreateRequest {
return &admin.ExecutionCreateRequest{
Project: executionConfig.TargetProject,
Domain: executionConfig.TargetDomain,
Expand Down Expand Up @@ -138,7 +146,8 @@ func resolveOverrides(toBeOverridden *ExecutionConfig, project string, domain st
func readConfigAndValidate(project string, domain string) (ExecutionParams, error) {
executionParams := ExecutionParams{}
if executionConfig.ExecFile == "" && executionConfig.Relaunch == "" {
return executionParams, fmt.Errorf("executionConfig or relaunch can't be empty. Run the flytectl get task/launchplan to generate the config")
return executionParams, fmt.Errorf("executionConfig or relaunch can't be empty." +
" Run the flytectl get task/launchplan to generate the config")
}
if executionConfig.Relaunch != "" {
resolveOverrides(executionConfig, project, domain)
Expand All @@ -155,7 +164,8 @@ func readConfigAndValidate(project string, domain string) (ExecutionParams, erro
isTask := readExecutionConfig.Task != ""
isWorkflow := readExecutionConfig.Workflow != ""
if isTask == isWorkflow {
return executionParams, fmt.Errorf("either one of task or workflow name should be specified to launch an execution")
return executionParams, fmt.Errorf("either one of task or workflow name should be specified" +
" to launch an execution")
}
name := readExecutionConfig.Task
execType := Task
Expand Down
3 changes: 2 additions & 1 deletion flytectl/cmd/delete/delete.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,8 @@ func RemoteDeleteCommand() *cobra.Command {
Long: deleteCmdLong,
}
terminateResourcesFuncs := map[string]cmdcore.CommandEntry{
"execution": {CmdFunc: terminateExecutionFunc, Aliases: []string{"executions"}, Short: execCmdShort, Long: execCmdLong},
"execution": {CmdFunc: terminateExecutionFunc, Aliases: []string{"executions"}, Short: execCmdShort,
Long: execCmdLong},
}
cmdcore.AddCommands(deleteCmd, terminateResourcesFuncs)
return deleteCmd
Expand Down
12 changes: 6 additions & 6 deletions flytectl/cmd/get/execution.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,12 @@ package get
import (
"context"

"github.com/flyteorg/flyteidl/gen/pb-go/flyteidl/admin"
"github.com/flyteorg/flytestdlib/logger"
"github.com/golang/protobuf/proto"

"github.com/flyteorg/flytectl/cmd/config"
cmdCore "github.com/flyteorg/flytectl/cmd/core"
"github.com/flyteorg/flytectl/pkg/printer"
"github.com/flyteorg/flyteidl/gen/pb-go/flyteidl/admin"
"github.com/flyteorg/flytestdlib/logger"
"github.com/golang/protobuf/proto"
)

const (
Expand Down Expand Up @@ -69,7 +68,7 @@ func getExecutionFunc(ctx context.Context, args []string, cmdCtx cmdCore.Command
var executions []*admin.Execution
if len(args) > 0 {
name := args[0]
execution, err := DefaultFetcher.FetchExecution(ctx, name, config.GetConfig().Project, config.GetConfig().Domain, cmdCtx)
execution, err := cmdCtx.AdminFetcherExt().FetchExecution(ctx, name, config.GetConfig().Project, config.GetConfig().Domain)
if err != nil {
return err
}
Expand All @@ -88,7 +87,8 @@ func getExecutionFunc(ctx context.Context, args []string, cmdCtx cmdCore.Command
executions = executionList.Executions
}
logger.Infof(ctx, "Retrieved %v executions", len(executions))
err := adminPrinter.Print(config.GetConfig().MustOutputFormat(), executionColumns, ExecutionToProtoMessages(executions)...)
err := adminPrinter.Print(config.GetConfig().MustOutputFormat(), executionColumns,
ExecutionToProtoMessages(executions)...)
if err != nil {
return err
}
Expand Down
16 changes: 0 additions & 16 deletions flytectl/cmd/get/execution_util.go
Original file line number Diff line number Diff line change
@@ -1,13 +1,11 @@
package get

import (
"context"
"errors"
"fmt"
"io/ioutil"
"os"

cmdCore "github.com/flyteorg/flytectl/cmd/core"
cmdUtil "github.com/flyteorg/flytectl/pkg/commandutils"
"github.com/flyteorg/flyteidl/clients/go/coreutils"
"github.com/flyteorg/flyteidl/gen/pb-go/flyteidl/admin"
Expand All @@ -29,20 +27,6 @@ type ExecutionConfig struct {
Inputs map[string]interface{} `json:"inputs"`
}

func (f FetcherImpl) FetchExecution(ctx context.Context, name string, project string, domain string, cmdCtx cmdCore.CommandContext) (*admin.Execution, error) {
e, err := cmdCtx.AdminClient().GetExecution(ctx, &admin.WorkflowExecutionGetRequest{
Id: &core.WorkflowExecutionIdentifier{
Project: project,
Domain: domain,
Name: name,
},
})
if err != nil {
return nil, err
}
return e, nil
}

func WriteExecConfigToFile(executionConfig ExecutionConfig, fileName string) error {
d, err := yaml.Marshal(executionConfig)
if err != nil {
Expand Down
90 changes: 90 additions & 0 deletions flytectl/cmd/get/execution_util_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
package get

import (
"testing"

"github.com/flyteorg/flyteidl/gen/pb-go/flyteidl/admin"
"github.com/flyteorg/flyteidl/gen/pb-go/flyteidl/core"
"github.com/stretchr/testify/assert"

"google.golang.org/protobuf/types/known/timestamppb"
)

func TestTaskInputs(t *testing.T) {
taskInputs := map[string]*core.Variable{}
t.Run("nil task", func(t *testing.T) {
retValue := TaskInputs(nil)
assert.Equal(t, taskInputs, retValue)
})
t.Run("valid inputs", func(t *testing.T) {
task := createTask()
retValue := TaskInputs(task)
assert.Equal(t, task.Closure.CompiledTask.Template.Interface.Inputs.Variables, retValue)
})
t.Run("closure compiled task nil", func(t *testing.T) {
task := createTask()
task.Closure.CompiledTask = nil
retValue := TaskInputs(task)
assert.Equal(t, taskInputs, retValue)
})
t.Run("closure compiled task template nil", func(t *testing.T) {
task := createTask()
task.Closure.CompiledTask.Template = nil
retValue := TaskInputs(task)
assert.Equal(t, taskInputs, retValue)
})
t.Run("closure compiled task template interface nil", func(t *testing.T) {
task := createTask()
task.Closure.CompiledTask.Template.Interface = nil
retValue := TaskInputs(task)
assert.Equal(t, taskInputs, retValue)
})
t.Run("closure compiled task template interface input nil", func(t *testing.T) {
task := createTask()
task.Closure.CompiledTask.Template.Interface.Inputs = nil
retValue := TaskInputs(task)
assert.Equal(t, taskInputs, retValue)
})
}

func createTask() *admin.Task {
sortedListLiteralType := core.Variable{
Type: &core.LiteralType{
Type: &core.LiteralType_CollectionType{
CollectionType: &core.LiteralType{
Type: &core.LiteralType_Simple{
Simple: core.SimpleType_INTEGER,
},
},
},
},
}

variableMap := map[string]*core.Variable{
"sorted_list1": &sortedListLiteralType,
"sorted_list2": &sortedListLiteralType,
}

inputs := &core.VariableMap{
Variables: variableMap,
}
typedInterface := &core.TypedInterface{
Inputs: inputs,
}
taskTemplate := &core.TaskTemplate{
Interface: typedInterface,
}
compiledTask := &core.CompiledTask{
Template: taskTemplate,
}
return &admin.Task{
Id: &core.Identifier{
Name: "task1",
Version: "v2",
},
Closure: &admin.TaskClosure{
CreatedAt: &timestamppb.Timestamp{Seconds: 1, Nanos: 0},
CompiledTask: compiledTask,
},
}
}
12 changes: 0 additions & 12 deletions flytectl/cmd/get/get.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package get

import (
cmdcore "github.com/flyteorg/flytectl/cmd/core"
"github.com/flyteorg/flytectl/cmd/get/interfaces"

"github.com/spf13/cobra"
)
Expand All @@ -18,17 +17,6 @@ Example get projects.
`
)

var (
DefaultFetcher = NewFetcherImpl()
)

func NewFetcherImpl() interfaces.Fetcher {
return FetcherImpl{}
}

type FetcherImpl struct {
}

// CreateGetCommand will return get command
func CreateGetCommand() *cobra.Command {
getCmd := &cobra.Command{
Expand Down
16 changes: 0 additions & 16 deletions flytectl/cmd/get/interfaces/fetcher_interface.go

This file was deleted.

Loading

0 comments on commit a0632f0

Please sign in to comment.