Skip to content

Commit

Permalink
fix: enclave manager passing in deprecated args object (#2342)
Browse files Browse the repository at this point in the history
## Description
The enclave builder would fail to render packages that used `def
run(plan, args)` where starlark in the package depended on values in
`args` being set. This was because the enclave manager was passing args
to interpretation using via an `args`. The use of `args` dict as sole
input is deprecated.

While it's deprecated we still support it for backwards compatibility
but everything in `args` is dropped - which caused the failure. This PR
fixes it by unpacking the values in `args` so enclave builder passes
them as unpacked `kwargs` to interpretation, as opposed to using the
sole deprecated `args` dict.

I've also added a test to highlight this behavior.

```
def run(plan, args):
	all_arg_values = args["arg1"] + ":" + args["arg2"]
	return all_arg_values
```
will error when passing input with only `args`
`{"args": {"arg1": "arg1-value", "arg2": "arg2-value"}}`


## Is this change user facing?
NO
  • Loading branch information
tedim52 authored Mar 27, 2024
1 parent 0dca278 commit 2bba1bc
Show file tree
Hide file tree
Showing 2 changed files with 34 additions and 1 deletion.
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,31 @@ def my_func(my_arg1, my_arg2, args):
require.Equal(suite.T(), expectedResult, result)
}

func (suite *StartosisInterpreterTestSuite) TestStartosisInterpreter_MainFunctionAndParamsErrWhenOnlyDeprecatedArgsObjectProvided() {
script := `
def run(plan, args):
all_arg_values = args["arg1"] + ":" + args["arg2"]
return all_arg_values
`
mainFunctionName := "run"
// passing in only "args" dictionary is deprecated
inputArgs := `{"args": {"arg1": "arg1-value", "arg2": "arg2-value"}}`

_, _, interpretationError := suite.interpreter.Interpret(context.Background(), startosis_constants.PackageIdPlaceholderForStandaloneScript, mainFunctionName, noPackageReplaceOptions, startosis_constants.PlaceHolderMainFileForPlaceStandAloneScript, script, inputArgs, defaultNonBlockingMode, emptyEnclaveComponents, emptyInstructionsPlanMask, defaultImageDownloadMode)
require.NotNil(suite.T(), interpretationError)
require.Equal(suite.T(), "Evaluation error: key \"arg1\" not in dict\n\tat [3:23]: run", interpretationError.GetErrorMessage())

scriptWithKwarg := `
def run(plan, arg0, args):
all_arg_values = arg0 + args["arg1"] + ":" + args["arg2"]
return all_arg_values
`
// however passing in kwargs, with args at the end is still fine
inputArgsWithKwarg := `{"arg0": "foo", "args": {"arg1": "arg1-value", "arg2": "arg2-value"}}`
_, _, interpretationError = suite.interpreter.Interpret(context.Background(), startosis_constants.PackageIdPlaceholderForStandaloneScript, mainFunctionName, noPackageReplaceOptions, startosis_constants.PlaceHolderMainFileForPlaceStandAloneScript, scriptWithKwarg, inputArgsWithKwarg, defaultNonBlockingMode, emptyEnclaveComponents, emptyInstructionsPlanMask, defaultImageDownloadMode)
require.Nil(suite.T(), interpretationError)
}

func (suite *StartosisInterpreterTestSuite) TestStartosisInterpreter_Test() {
script := `
def run(plan):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ export const KurtosisPackageNode = memo(

useEffect(() => {
const packageId = nodeData?.packageId;
const args = nodeData?.args;
let args = nodeData?.args;
if (isDefined(packageId) && isDefined(args) && packageId !== "") {
let cancelled = false;
(async () => {
Expand All @@ -40,6 +40,14 @@ export const KurtosisPackageNode = memo(
setMode({ type: "error", error: "APIC info missing from temporary enclave" });
return;
}

// If args only has one record, and its value is args, ASSUME user is passing args via a JSON or YAML into the args object of def run(plan, args)
// via Json or Yaml editor
// If only an `args` object is provided, kurtosis will not interpret the value in the args object as passing args via the args dictionary is (technically) deprecated even though it's still allowed
if (Object.keys(args).length === 1 && args.hasOwnProperty("args")) {
args = args["args"] as Record<string, string>; // TODO(tedi): ideally we'd validate and handle this in transform args utils
}

const plan = await kurtosisClient.getStarlarkPackagePlanYaml(
enclave.value.enclaveInfo.apiContainerInfo,
packageId,
Expand Down

0 comments on commit 2bba1bc

Please sign in to comment.