feat: impl dependency list flag #2543
Conversation
| func (enclaveCtx *EnclaveContext) GetStarlarkPackagePlanYaml(ctx context.Context, packageId string, serializedParams string, dependenciesOnly bool) (*kurtosis_core_rpc_api_bindings.PlanYaml, error) { | ||
| serializedParams, err := maybeParseYaml(serializedParams) | ||
| if err != nil { | ||
| return nil, stacktrace.Propagate(err, "An error occurred when parsing YAML args for package '%v'", serializedParams) |
There was a problem hiding this comment.
should this be s/serializedParams/packageId/?
| DependenciesOnly: &dependenciesOnly, | ||
| }) | ||
| if err != nil { | ||
| return nil, stacktrace.Propagate(err, "An error occurred while getting the starlark package plan yaml run.") |
There was a problem hiding this comment.
Is the "....run" at the end of the sentence perhaps a copy-paste that wasn't deleted? (my understanding is this returns the actual YAML; feel free to ignore this comment if not useful)
| DependenciesOnly: &dependenciesOnly, | ||
| }) | ||
| if err != nil { | ||
| return nil, stacktrace.Propagate(err, "An error occurred while getting the last starlark script plan yaml run.") |
| @@ -55,6 +60,11 @@ const ( | |||
| dryRunFlagKey = "dry-run" | |||
| defaultDryRun = "false" | |||
|
|
|||
| dependenciesFlagKey = "dependencies" | |||
| defaultDependencies = "false" | |||
There was a problem hiding this comment.
suggestion to make this dependenciesFlagDefault, both to pattern match other varnames and not suggest that this variable is in fact containing a list of default dependencies
| dependenciesFlagKey = "dependencies" | ||
| defaultDependencies = "false" | ||
| pullDependenciesFlagKey = "pull" | ||
| defaultPullDependencies = "false" |
| @@ -386,6 +391,16 @@ func (interpreter *StartosisInterpreter) buildBindings( | |||
| return &predeclared, nil | |||
| } | |||
|
|
|||
| const ( | |||
| moduleIdSeperator = "/" | |||
There was a problem hiding this comment.
Thought: wouldn't this constant already be declared somewhere in the Startosis engine? Should we use that instead, so we don't have two constants doing the same thing?
| ) | ||
|
|
||
| func getModulePrefix(moduleId string) string { | ||
| return strings.Join(strings.SplitN(moduleId, moduleIdSeperator, numModuleIdSeparators)[:prefixNum], moduleIdSeperator) |
There was a problem hiding this comment.
From what I understand of SplitN, isn't numModuleIdSeparators always just prefixNum + 1 (so we don't need to maintain the extra const)?
| @@ -115,7 +115,7 @@ func (suite *StartosisIntepreterPlanYamlTestSuite) TestAddServiceWithFilesArtifa | |||
| require.Nil(suite.T(), interpretationError) | |||
There was a problem hiding this comment.
Should this file be renamed to startosis_interpreter_plan_yaml_generator_test to match the name of PlanYaml -> PlanYamlGenerator?
| @@ -813,6 +833,9 @@ tasks: | |||
| command: | |||
| - echo {{ kurtosis.4.code }} {{ kurtosis.4.output }} | |||
| image: badouralix/curl-jq | |||
| images: | |||
| - badouralix/curl-jq | |||
| - postgres:latest | |||
| ` | |||
| require.Equal(suite.T(), expectedYaml, planYaml) | |||
| } | |||
There was a problem hiding this comment.
Should there be a test case here where the dependenciesOnly flag is true? (assuming that, per the comment above, we maintain the dependenciesOnly flag on the PlanYamlGenerator and it's not the responsibility of the CLI or user to pull out the info they want)
| prefixNum = 3 | ||
| ) | ||
|
|
||
| func getModulePrefix(moduleId string) string { |
There was a problem hiding this comment.
Reviewing this, it'd have been helpful to have a comment giving an example of getModulePrefix is supposed to return so that I know how to model its output in the above code
Description
kurtosis run github.com/kurtosis-tech/... --dependenciesreturns ayamlwith a list of images and packages the run will need to depend on.Adding
--pullwill pull all the dependencies locally. Thekurtosis.ymlwill be updated to use local versions of packages and Docker images are pulled onto the machine locallyIs this change user facing?
YES