Conversion fixes, add conversion for background step, gitClone step and runtime/infra in CI stages.#312
Conversation
…ing of days and weeks when unmarshaling json/yaml
…delegateSelector and failureStrategy conversion
… omit empty values
… conversion to use an empty template for step types whose conversion is yet to be implemented
…late name for k8s cananry deploy step
…nce, and k8s runtime
…ia conversion for approval steps
… build and push step, add type check for executeOnDelegate in notification conversion
…cations converters
…y flexible.Field Unmarshalling
Ompragash
left a comment
There was a problem hiding this comment.
Thanks for the PR! Took a thorough look at the changes. Solid work overall. Found a few things that need to be addressed before we merge this in:
Must-fix:
-
StepGitCloneYAML tags are broken -Repositoryhas a typo (yaml:"repoNama"→yaml:"repoName"), andDepth,SSLVerify, andTimeoutall share the sameyaml:"cloneDirectory"tag. Each needs its own correct tag (yaml:"depth",yaml:"sslVerify",yaml:"timeout"). This will silently break YAML unmarshalling for those fields. -
Envtype mismatch inConvertStepBackground-StepBackground.Envis*flexible.Field[map[string]interface{}]but it's being assigned directly tov1.StepRun.Env. This needs proper type conversion. -
Nil pointer risk in
ConvertStepGitClone- ifbuild.Specis nil, accessingbuild.Spec.Branchetc. will panic. Add a nil guard forbuild.Spec. -
Non-deterministic port ordering - iterating over
sp.PortBindingsmap produces random ordering. Sort the keys first to ensure reproducible YAML output and stable tests. -
StepIACMOpenTofuPluginis missingCommonStepSpecembedding -StepIACMTerraformPluginhas it butStepIACMOpenTofuPlugindoesn't. Looks like an oversight since they're structurally identical otherwise.
Cleanup:
- Missing Apache 2.0 license headers on
convert_step_git_clone.go,convert_output_variables.go, andstep_iacm.go - Mixed tabs and spaces in
ConvertServiceDependencyToBackgroundStep,ConvertServiceDependenciesToBackgroundSteps, andConvertSharedPaths- rungofmt - The comment
// Convert size - map "flex" to "xlarge" as per your exampleinConvertRuntimeis misleading since the code does a direct pass-through ConvertOutputVariablesreturns an empty slice[]instead ofnilwhen there are no outputs, which will produceoutputs: []in YAML instead of omitting the field- Hardcoding
shell: "sh"as default inConvertStepBackgroundmight not be ideal - consider leaving it empty so v1 uses its own default Infrastructure,Runtime, andVolumeonly haveUnmarshalJSONbut noUnmarshalYAML- if pipeline inputs come as YAML (which they usually do), these won't deserialize correctly
Tests needed:
convert_step_background_test.go- cover nil input, minimal step (image only), full step with ports/reports/resources/shell/env/entrypoint, and edge cases like empty command, unknown shell valuesconvert_step_git_clone_test.go- cover nil input, branch build type, tag build type, PR build type, commitSha build type, nilbuild.Spec, and all optional fields (depth, sslVerify, cloneDirectory)convert_output_variables_test.go- cover nil/empty input, single output, multiple outputs, output withType: "Secret"(mask=true), output with value vs name fallbackconvert_stage_ci_test.go- tests forConvertRuntime(Cloud, Docker, unknown type),ConvertInfrastructureToRuntime(KubernetesDirect with full spec, VM with pool, nil infra),ConvertServiceDependencyToBackgroundStep,ConvertServiceDependenciesToBackgroundSteps,ConvertSharedPaths,ConvertInfrastructureToVolumes(all volume types: EmptyDir, PVC, HostPath, ConfigMap, Secret)infrastructure_test.go/runtime_test.go- JSON unmarshalling tests for the new v0 types to make sureUnmarshalJSONdispatch works correctly for each type variant
Ompragash
left a comment
There was a problem hiding this comment.
Merging it for now with Harsh's approval.
|
@peroxidemonke7 Let's ensure to address the quality gaps incrementally, as this repo is used for Jenkins migration-related tasks |
No description provided.