feat: Make Run also accept argument other than args dict (#859)

## Description:
<!-- Describe this change, how it works, and the motivation behind it.
-->

## Is this change user facing?
YES
<!-- If yes, please add the "user facing" label to the PR -->
<!-- If yes, don't forget to include docs changes where relevant -->

## References (if applicable):
<!-- Add relevant Github Issues, Discord threads, or other helpful
information. -->

.github/workflows/golangci-lint.yml

@@ -10,12 +10,18 @@ concurrency:
   group: "go-linter-${{ github.ref }}"
   cancel-in-progress: true
 
+permissions:
+  contents: read
+
 on:
   push:
     branches:
       - gh-readonly-queue/main/**
   pull_request:
 
+env:
+  GOLINT_VERSION: v1.53.3
+
 jobs:
   golangci:
     name: golang-lint
@@ -30,77 +36,77 @@ jobs:
       - name: lint-container-engine-lib
         uses: golangci/golangci-lint-action@v3
         with:
-          version: v1.49.0
+          version: ${{ env.GOLINT_VERSION }}
           working-directory: container-engine-lib/
           args: --timeout=3m
           skip-pkg-cache: true
       - name: lint-contexts-config-store
         uses: golangci/golangci-lint-action@v3
         with:
-          version: v1.49.0
+          version: ${{ env.GOLINT_VERSION }}
           working-directory: contexts-config-store/
           args: --timeout=3m
           skip-pkg-cache: true
       - name: lint-grpc-file-transfer
         uses: golangci/golangci-lint-action@v3
         with:
-          version: v1.49.0
+          version: ${{ env.GOLINT_VERSION }}
           working-directory: grpc-file-transfer/golang/
           args: --timeout=3m
           skip-pkg-cache: true
       - name: lint-name-generator
         uses: golangci/golangci-lint-action@v3
         with:
-          version: v1.49.0
+          version: ${{ env.GOLINT_VERSION }}
           working-directory: name_generator/
           args: --timeout=3m
           skip-pkg-cache: true
       - name: lint-api-golang
         uses: golangci/golangci-lint-action@v3
         with:
-          version: v1.49.0
+          version: ${{ env.GOLINT_VERSION }}
           working-directory: api/golang/
           args: --timeout=3m
           skip-pkg-cache: true
       - name: lint-core-server
         uses: golangci/golangci-lint-action@v3
         with:
-          version: v1.49.0
+          version: ${{ env.GOLINT_VERSION }}
           working-directory: core/server
           args: --timeout=3m
           skip-pkg-cache: true
       - name: lint-core-launcher
         uses: golangci/golangci-lint-action@v3
         with:
-          version: v1.49.0
+          version: ${{ env.GOLINT_VERSION }}
           working-directory: core/launcher/
           args: --timeout=3m
           skip-pkg-cache: true
       - name: lint-engine-server
         uses: golangci/golangci-lint-action@v3
         with:
-          version: v1.49.0
+          version: ${{ env.GOLINT_VERSION }}
           working-directory: engine/server/
           args: --timeout=3m
           skip-pkg-cache: true
       - name: lint-engine-launcher
         uses: golangci/golangci-lint-action@v3
         with:
-          version: v1.49.0
+          version: ${{ env.GOLINT_VERSION }}
           working-directory: engine/launcher/
           args: --timeout=3m
           skip-pkg-cache: true
       - name: lint-internal-test-suites
         uses: golangci/golangci-lint-action@v3
         with:
-          version: v1.49.0
+          version: ${{ env.GOLINT_VERSION }}
           working-directory: internal_testsuites/golang/
           args: --timeout=3m
           skip-pkg-cache: true
       - name: lint-cli
         uses: golangci/golangci-lint-action@v3
         with:
-          version: v1.49.0
+          version: ${{ env.GOLINT_VERSION }}
           working-directory: cli/cli/
           args: --timeout=3m
           skip-pkg-cache: true

cli/cli/commands/service/add/add.go

@@ -307,7 +307,7 @@ func run(
 	plan.add_service(name = "%s", config = %s)
 	plan.print("%s") # we add this print of a random UUID to make sure the single add_service above won't get cached
 `, serviceName, serviceConfigStarlark, uuid.New().String())
-	starlarkRunResult, err := enclaveCtx.RunStarlarkScriptBlocking(ctx, useDefaultMainFile, starlarkScript, "", false, defaultParallelism, noExperimentalFeature)
+	starlarkRunResult, err := enclaveCtx.RunStarlarkScriptBlocking(ctx, useDefaultMainFile, starlarkScript, "{}", false, defaultParallelism, noExperimentalFeature)
 	if err != nil {
 		return stacktrace.Propagate(err, "An error has occurred when running Starlark to add service")
 	}

core/server/api_container/server/startosis_engine/startosis_interpreter.go

@@ -263,17 +263,14 @@ func (interpreter *StartosisInterpreter) Interpret(
 
 	runFunctionExecutionThread := newStarlarkThread(starlarkGoThreadName)
 
-	if isUsingDefaultMainFunction && mainFunction.NumParams() > maximumParamsAllowedForRunFunction {
-		return startosis_constants.NoOutputObject, nil, startosis_errors.NewInterpretationError("The 'run' entrypoint function can have at most '%v' argument got '%v'", maximumParamsAllowedForRunFunction, mainFunction.NumParams()).ToAPIType()
-	}
-
 	var argsTuple starlark.Tuple
 	var kwArgs []starlark.Tuple
 
 	mainFuncParamsNum := mainFunction.NumParams()
 
+	// The plan object will always be injected if the first argument name is 'plan'
+	// If we are on main, 'plan' must be the first argument
 	if mainFuncParamsNum >= minimumParamsRequiredForPlan {
-		// the plan object will always be injected if the first argument name is 'plan'
 		firstParamName, _ := mainFunction.Param(planParamIndex)
 		if firstParamName == planParamName {
 			kurtosisPlanInstructions := KurtosisPlanInstructions(interpreter.serviceNetwork, interpreter.recipeExecutor, interpreter.moduleContentProvider)
@@ -286,28 +283,27 @@ func (interpreter *StartosisInterpreter) Interpret(
 		}
 	}
 
-	if (isUsingDefaultMainFunction && mainFuncParamsNum == paramsRequiredForArgs) ||
-		(!isUsingDefaultMainFunction && mainFuncParamsNum > 0) {
-		if isUsingDefaultMainFunction {
-			if paramName, _ := mainFunction.Param(argsParamIndex); paramName != argsParamName {
-				return startosis_constants.NoOutputObject, nil, startosis_errors.NewInterpretationError(unexpectedArgNameError, argsParamIndex, argsParamName, paramName).ToAPIType()
-			}
-		}
-		// run function has an argument so we parse input args
-		inputArgs, interpretationError := interpreter.parseInputArgs(runFunctionExecutionThread, serializedJsonParams)
-		if interpretationError != nil {
-			return startosis_constants.NoOutputObject, nil, interpretationError.ToAPIType()
-		}
-		if isUsingDefaultMainFunction {
+	inputArgs, interpretationError := interpreter.parseInputArgs(runFunctionExecutionThread, serializedJsonParams)
+	if interpretationError != nil {
+		return startosis_constants.NoOutputObject, nil, interpretationError.ToAPIType()
+	}
+
+	// For backwards compatibility, deal with case run(plan, args), where args is a generic dictionary
+	runWithGenericDictArgs := false
+	if isUsingDefaultMainFunction && mainFuncParamsNum == paramsRequiredForArgs {
+		if paramName, _ := mainFunction.Param(argsParamIndex); paramName == argsParamName {
+			logrus.Warnf("Using args dictionary as parameter is deprecated. Consider unpacking the dictionary into individual parameters. For example: run(plan, args) to run(plan, param1, param2, ...)")
 			argsTuple = append(argsTuple, inputArgs)
 			kwArgs = noKwargs
-		} else {
-			argsDict, ok := inputArgs.(*starlark.Dict)
-			if !ok {
-				return startosis_constants.NoOutputObject, nil, startosis_errors.NewInterpretationError("An error occurred casting input args '%s' to Starlark Dict", inputArgs).ToAPIType()
-			}
-			kwArgs = append(kwArgs, argsDict.Items()...)
+			runWithGenericDictArgs = true
+		}
+	}
+	if !runWithGenericDictArgs {
+		argsDict, ok := inputArgs.(*starlark.Dict)
+		if !ok {
+			return startosis_constants.NoOutputObject, nil, startosis_errors.NewInterpretationError("An error occurred casting input args '%s' to Starlark Dict", inputArgs).ToAPIType()
 		}
+		kwArgs = append(kwArgs, argsDict.Items()...)
 	}
 
 	outputObject, err := starlark.Call(runFunctionExecutionThread, mainFunction, argsTuple, kwArgs)

core/server/api_container/server/startosis_engine/startosis_interpreter_test.go

@@ -942,7 +942,7 @@ def run(plan):
 	require.Nil(t, instructionsPlan)
 }
 
-func TestStartosisInterpreter_RunWithoutArgsNoArgsPassed(t *testing.T) {
+func TestStartosisInterpreter_RunWithoutArgsAndNoArgsPassed(t *testing.T) {
 	packageContentProvider := mock_package_content_provider.NewMockPackageContentProvider()
 	defer packageContentProvider.RemoveAll()
 	runtimeValueStore := runtime_value_store.NewRuntimeValueStore()
@@ -961,7 +961,7 @@ def run(plan):
 	validateScriptOutputFromPrintInstructions(t, instructionsPlan, expectedOutput)
 }
 
-func TestStartosisInterpreter_RunWithoutArgsArgsPassed(t *testing.T) {
+func TestStartosisInterpreter_RunWithoutArgsAndArgsPassed(t *testing.T) {
 	packageContentProvider := mock_package_content_provider.NewMockPackageContentProvider()
 	defer packageContentProvider.RemoveAll()
 	runtimeValueStore := runtime_value_store.NewRuntimeValueStore()
@@ -972,15 +972,11 @@ def run(plan):
 `
 
 	_, instructionsPlan, interpretationError := interpreter.Interpret(context.Background(), startosis_constants.PackageIdPlaceholderForStandaloneScript, useDefaultMainFunctionName, script, `{"number": 4}`, emptyInstructionsPlanMask)
-	require.Nil(t, interpretationError)
-	require.Equal(t, 1, instructionsPlan.Size())
-
-	expectedOutput := `Hello World!
-`
-	validateScriptOutputFromPrintInstructions(t, instructionsPlan, expectedOutput)
+	require.NotNil(t, interpretationError)
+	require.Nil(t, instructionsPlan)
 }
 
-func TestStartosisInterpreter_RunWithArgsArgsPassed(t *testing.T) {
+func TestStartosisInterpreter_RunWithArgsAndArgsPassed(t *testing.T) {
 	packageContentProvider := mock_package_content_provider.NewMockPackageContentProvider()
 	defer packageContentProvider.RemoveAll()
 	runtimeValueStore := runtime_value_store.NewRuntimeValueStore()
@@ -999,7 +995,7 @@ def run(plan, args):
 	validateScriptOutputFromPrintInstructions(t, instructionsPlan, expectedOutput)
 }
 
-func TestStartosisInterpreter_RunWithArgsNoArgsPassed(t *testing.T) {
+func TestStartosisInterpreter_RunWithArgsAndNoArgsPassed(t *testing.T) {
 	packageContentProvider := mock_package_content_provider.NewMockPackageContentProvider()
 	defer packageContentProvider.RemoveAll()
 	runtimeValueStore := runtime_value_store.NewRuntimeValueStore()
@@ -1033,11 +1029,46 @@ def run(plan, args, invalid_arg):
 
 	_, instructionsPlan, interpretationError := interpreter.Interpret(context.Background(), startosis_constants.PackageIdPlaceholderForStandaloneScript, useDefaultMainFunctionName, script, startosis_constants.EmptyInputArgs, emptyInstructionsPlanMask)
 	require.NotNil(t, interpretationError)
-	expectedError := fmt.Sprintf("The 'run' entrypoint function can have at most '%v' argument got '%v'", maximumParamsAllowedForRunFunction, 3)
-	require.Equal(t, expectedError, interpretationError.GetErrorMessage())
+	expectedError := "Evaluation error: function run missing 2 arguments (args, invalid_arg)"
+	require.Contains(t, interpretationError.GetErrorMessage(), expectedError)
 	require.Nil(t, instructionsPlan)
 }
 
+func TestStartosisInterpreter_RunWithUnpackedDictButMissingArgs(t *testing.T) {
+	packageContentProvider := mock_package_content_provider.NewMockPackageContentProvider()
+	defer packageContentProvider.RemoveAll()
+	runtimeValueStore := runtime_value_store.NewRuntimeValueStore()
+	interpreter := NewStartosisInterpreter(testServiceNetwork, packageContentProvider, runtimeValueStore)
+	script := `
+def run(plan, a, b):
+	plan.print("this wouldn't interpret so the text here doesnt matter")
+`
+	missingArgumentCount := 1
+	missingArgument := "b"
+	_, instructionsPlan, interpretationError := interpreter.Interpret(context.Background(), startosis_constants.PackageIdPlaceholderForStandaloneScript, useDefaultMainFunctionName, script, `{"a": "x"}`, emptyInstructionsPlanMask)
+	require.NotNil(t, interpretationError)
+
+	expectedError := fmt.Sprintf("Evaluation error: function run missing %d argument (%v)", missingArgumentCount, missingArgument)
+	require.Contains(t, interpretationError.GetErrorMessage(), expectedError)
+	require.Nil(t, instructionsPlan)
+}
+
+func TestStartosisInterpreter_RunWithUnpackedDict(t *testing.T) {
+	packageContentProvider := mock_package_content_provider.NewMockPackageContentProvider()
+	defer packageContentProvider.RemoveAll()
+	runtimeValueStore := runtime_value_store.NewRuntimeValueStore()
+	interpreter := NewStartosisInterpreter(testServiceNetwork, packageContentProvider, runtimeValueStore)
+	script := `
+def run(plan, a, b=1):
+	plan.print("My favorite number is {0}, but my favorite letter is {1}".format(b, a))
+`
+	_, instructionsPlan, interpretationError := interpreter.Interpret(context.Background(), startosis_constants.PackageIdPlaceholderForStandaloneScript, useDefaultMainFunctionName, script, `{"a": "x"}`, emptyInstructionsPlanMask)
+	require.Nil(t, interpretationError)
+	require.Equal(t, 1, instructionsPlan.Size())
+	expectedOutput := "My favorite number is 1, but my favorite letter is x\n"
+	validateScriptOutputFromPrintInstructions(t, instructionsPlan, expectedOutput)
+}
+
 func TestStartosisInterpreter_PrintWithoutPlanErrorsNicely(t *testing.T) {
 	packageContentProvider := mock_package_content_provider.NewMockPackageContentProvider()
 	defer packageContentProvider.RemoveAll()

docs/docs/concepts-reference/args.md

@@ -14,7 +14,7 @@ def run(plan):
 to:
 
 ```python
-def run(plan, args)
+def run(plan, some_other_param, some_parameter="Default value")
 ```
 
 Then pass JSON-serialized arg values to `kurtosis run` in the CLI. For example:
@@ -25,13 +25,5 @@ kurtosis run github.com/USERNAME/REPO '{"some_parameter":"some_value","some_othe
 
 Kurtosis will automatically JSON-deserialize the JSON string, and then pass it in to the `run` function in Starlark.
 
-The JSON passed in via the command line will be deserialized to a dictionary in Starlark (_not_ a `struct`). So to access the args above, your `main.star` might look like:
-
-```python
-def run(plan, args):
-    plan.print("some_parameter value: " + args["some_parameter"])
-    plan.print("some_other_param value: " + args["some_other_param"])
-```
-
 <!------------------------------------- ONLY LINKS BELOW HERE --------------------------------->
 [packages-reference]: ./packages.md

internal_testsuites/golang/test_helpers/test_helpers.go

@@ -163,7 +163,7 @@ func AddService(
 def run(plan):
 	plan.add_service(name = "%s", config = %s)
 `, serviceName, serviceConfigStarlark)
-	starlarkRunResult, err := enclaveCtx.RunStarlarkScriptBlocking(ctx, useDefaultMainFile, starlarkScript, "", false, defaultParallelism, noExperimentalFeature)
+	starlarkRunResult, err := enclaveCtx.RunStarlarkScriptBlocking(ctx, useDefaultMainFile, starlarkScript, "{}", false, defaultParallelism, noExperimentalFeature)
 	if err != nil {
 		return nil, stacktrace.Propagate(err, "An error has occurred when running Starlark to add service")
 	}