feat(script): allow setting file extension #13555
Conversation
Signed-off-by: Andrzej Ressel <[email protected]>
andrzejressel
requested review from
terrytangyuan and
sarabala1979
as code owners
agilgur5
changed the title
workflow/controller/operator.go
Outdated
| @@ -3088,7 +3088,8 @@ func (woc *wfOperationCtx) executeScript(ctx context.Context, nodeName string, t | |||
| if len(tmpl.Script.Source) == 0 { | |||
| woc.log.Warn("'script.source' is empty, suggest change template into 'container'") | |||
| } else { | |||
| mainCtr.Args = append(mainCtr.Args, common.ExecutorScriptSourcePath) | |||
| scriptSourcePath := common.GetExecutorScriptSourcePath(tmpl.Script.Extension) | |||
There was a problem hiding this comment.
I'm thinking this makes more sense as a receiver function on the template struct
| scriptSourcePath := common.GetExecutorScriptSourcePath(tmpl.Script.Extension) | |
| scriptSourcePath := tmpl.GetScriptSourcePath() |
There was a problem hiding this comment.
and this variable could just be in-lined instead then
There was a problem hiding this comment.
Fixed, but I had to move these 2 constants to Template - I don't know If it looks good ...
There was a problem hiding this comment.
You don't need to move the 2 constants, you can leave them in common and just import them from there. See further comments below
Signed-off-by: Andrzej Ressel <[email protected]>
Signed-off-by: Andrzej Ressel <[email protected]>
Signed-off-by: Andrzej Ressel <[email protected]>
Signed-off-by: Andrzej Ressel <[email protected]>
workflow/controller/operator.go
Outdated
| @@ -3088,7 +3088,8 @@ func (woc *wfOperationCtx) executeScript(ctx context.Context, nodeName string, t | |||
| if len(tmpl.Script.Source) == 0 { | |||
| woc.log.Warn("'script.source' is empty, suggest change template into 'container'") | |||
| } else { | |||
| mainCtr.Args = append(mainCtr.Args, common.ExecutorScriptSourcePath) | |||
| scriptSourcePath := common.GetExecutorScriptSourcePath(tmpl.Script.Extension) | |||
There was a problem hiding this comment.
You don't need to move the 2 constants, you can leave them in common and just import them from there. See further comments below
| @@ -1184,7 +1184,7 @@ func addScriptStagingVolume(pod *apiv1.Pod) { | |||
| if initCtr.Name == common.InitContainerName { | |||
| volMount := apiv1.VolumeMount{ | |||
| Name: volName, | |||
| MountPath: common.ExecutorStagingEmptyDir, | |||
| MountPath: wfv1.ExecutorStagingEmptyDir, | |||
There was a problem hiding this comment.
I think these make more sense to be in common if they're also used here
| ExecutorStagingEmptyDir = "/argo/staging" | ||
| // ExecutorScriptSourcePath is the path which init will write the script source file to for script templates | ||
| executorScriptSourcePath = "/argo/staging/script" | ||
| ) |
There was a problem hiding this comment.
You can just leave these in common and import them here
There was a problem hiding this comment.
There seems to be cyclic import
diff --git a/pkg/apis/workflow/v1alpha1/workflow_types.go b/pkg/apis/workflow/v1alpha1/workflow_types.go
index da02c87bd..55648ca8c 100644
--- a/pkg/apis/workflow/v1alpha1/workflow_types.go
+++ b/pkg/apis/workflow/v1alpha1/workflow_types.go
@@ -26,6 +26,7 @@ import (
argoerrs "github.com/argoproj/argo-workflows/v3/errors"
"github.com/argoproj/argo-workflows/v3/util/slice"
+ "github.com/argoproj/argo-workflows/v3/workflow/common"
)
// TemplateType is the type of a template
@@ -815,19 +816,12 @@ func (tmpl *Template) GetOutputs() *Outputs {
return nil
}
-const (
- // ExecutorStagingEmptyDir is the path of the emptydir which is used as a staging area to transfer a file between init/main container for script/resource templates
- ExecutorStagingEmptyDir = "/argo/staging"
- // ExecutorScriptSourcePath is the path which init will write the script source file to for script templates
- executorScriptSourcePath = "/argo/staging/script"
-)
-
func (tmpl *Template) GetScriptSourcePath() string {
extension := tmpl.Script.Extension
if len(extension) != 0 {
- return executorScriptSourcePath + "." + tmpl.Script.Extension
+ return common.ExecutorScriptSourcePath + "." + tmpl.Script.Extension
} else {
- return executorScriptSourcePath
+ return common.ExecutorScriptSourcePath
}
}
diff --git a/workflow/common/common.go b/workflow/common/common.go
index 55011c099..b4f174263 100644
--- a/workflow/common/common.go
+++ b/workflow/common/common.go
@@ -114,6 +114,10 @@ const (
// as well as artifact collection by the wait container.
ExecutorMainFilesystemDir = "/mainctrfs"
+ // ExecutorStagingEmptyDir is the path of the emptydir which is used as a staging area to transfer a file between init/main container for script/resource templates
+ ExecutorStagingEmptyDir = "/argo/staging"
+ // ExecutorScriptSourcePath is the path which init will write the script source file to for script templates
+ ExecutorScriptSourcePath = "/argo/staging/script"
// ExecutorResourceManifestPath is the path which init will write the manifest file to for resource templates
ExecutorResourceManifestPath = "/tmp/manifest.yaml"package github.com/argoproj/argo-workflows/v3/config
imports github.com/argoproj/argo-workflows/v3/pkg/apis/workflow/v1alpha1
imports github.com/argoproj/argo-workflows/v3/workflow/common
imports github.com/argoproj/argo-workflows/v3/pkg/apis/workflow/v1alpha1: import cycle not allowed
There was a problem hiding this comment.
I thought there might be, although I couldn't find one myself by visual inspection; it seems to import the workflow package (workflow/common.go) but not v1alpha1 🤔 Maybe I was on a stale branch
Ideally constant files shouldn't import anything so they're a single source of truth.
There was a problem hiding this comment.
Ah, other files in the common package do import v1alpha1 (ancestry.go, convert.go, parse.go, and util.go). It's potentially worth splitting off a constants package
Signed-off-by: Andrzej Ressel <[email protected]>
Motivation
Currently script file has no extension. Some intepreters however require specific extension - namely Java (at least in some situations) and Scala.
Modifications
New option in
scriptobject:extensionVerification