You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2021/12/29 15:01:45 UTC
[GitHub] [beam] AydarZaynutdinov commented on a change in pull request #16384: [BEAM-13308] [Playground] Getting baseFileFolder from environment
AydarZaynutdinov commented on a change in pull request #16384:
URL: https://github.com/apache/beam/pull/16384#discussion_r776356060
##########
File path: playground/backend/internal/code_processing/code_processing.go
##########
@@ -219,8 +219,8 @@ func getExecuteCmd(valRes *sync.Map, executor *executors.Executor, ctxWithTimeou
}
// setJavaExecutableFile sets executable file name to runner (JAVA class name is known after compilation step)
-func setJavaExecutableFile(lc *fs_tool.LifeCycle, id uuid.UUID, service cache.Cache, ctx context.Context, executorBuilder *executors.ExecutorBuilder, dir string) (executors.Executor, error) {
- className, err := lc.ExecutableName(id, dir)
+func setJavaExecutableFile(lc *fs_tool.LifeCycle, id uuid.UUID, service cache.Cache, ctx context.Context, executorBuilder *executors.ExecutorBuilder, dir, pipelinesFolder string) (executors.Executor, error) {
+ className, err := lc.ExecutableName(id, dir, pipelinesFolder)
Review comment:
```suggestion
className, err := lc.ExecutableName(id, filepath.Join(dir, pipelinesFolder))
```
##########
File path: playground/backend/internal/code_processing/code_processing_test.go
##########
@@ -242,7 +242,7 @@ func Test_Process(t *testing.T) {
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
- lc, _ := fs_tool.NewLifeCycle(pb.Sdk_SDK_JAVA, tt.args.pipelineId, os.Getenv("APP_WORK_DIR"))
+ lc, _ := fs_tool.NewLifeCycle(pb.Sdk_SDK_JAVA, tt.args.pipelineId, os.Getenv("APP_WORK_DIR"), pipelinesFolder)
Review comment:
```suggestion
lc, _ := fs_tool.NewLifeCycle(pb.Sdk_SDK_JAVA, tt.args.pipelineId, filepath.Join(os.Getenv("APP_WORK_DIR"), pipelinesFolder))
```
##########
File path: playground/backend/internal/code_processing/code_processing_test.go
##########
@@ -560,6 +561,7 @@ func Test_setJavaExecutableFile(t *testing.T) {
ctx: context.Background(),
executorBuilder: &executorBuilder,
dir: "",
+ pipelinesFolder: pipelinesFolder,
Review comment:
```suggestion
dir: pipelinesFolder,
```
##########
File path: playground/backend/internal/code_processing/code_processing_test.go
##########
@@ -677,14 +679,14 @@ func teardownBenchmarks() {
if err != nil {
panic(fmt.Errorf("error during test teardown: %s", err.Error()))
}
- err = os.RemoveAll(baseFileFolder)
+ err = os.RemoveAll(pipelinesFolder)
if err != nil {
panic(fmt.Errorf("error during test teardown: %s", err.Error()))
}
}
func prepareFiles(b *testing.B, pipelineId uuid.UUID, code string, sdk pb.Sdk) *fs_tool.LifeCycle {
- lc, err := fs_tool.NewLifeCycle(sdk, pipelineId, "")
+ lc, err := fs_tool.NewLifeCycle(sdk, pipelineId, "", pipelinesFolder)
Review comment:
We could use the old version with only one argument - `dir`. Method `NewLifeCycle ` shouldn't implement any logic like filepath.Join(dir, pipelinesFolder)
##########
File path: playground/backend/internal/fs_tool/fs.go
##########
@@ -51,20 +51,20 @@ type LifeCycle struct {
folderGlobs []string //folders that should be created to process code
Folder Folder
Extension Extension
- ExecutableName func(uuid.UUID, string) (string, error)
+ ExecutableName func(uuid.UUID, string, string) (string, error)
Review comment:
Do not need to send 2 arguments `(dir1, dir2)` and then use something like `filepath.Join(dir1, dir2)`
##########
File path: playground/backend/internal/fs_tool/fs.go
##########
@@ -51,20 +51,20 @@ type LifeCycle struct {
folderGlobs []string //folders that should be created to process code
Folder Folder
Extension Extension
- ExecutableName func(uuid.UUID, string) (string, error)
+ ExecutableName func(uuid.UUID, string, string) (string, error)
pipelineId uuid.UUID
}
// NewLifeCycle returns a corresponding LifeCycle depending on the given SDK.
// workingDir should be existed and be prepared to create/delete/modify folders into him.
-func NewLifeCycle(sdk pb.Sdk, pipelineId uuid.UUID, workingDir string) (*LifeCycle, error) {
+func NewLifeCycle(sdk pb.Sdk, pipelineId uuid.UUID, workingDir, pipelinesFolder string) (*LifeCycle, error) {
switch sdk {
case pb.Sdk_SDK_JAVA:
- return newJavaLifeCycle(pipelineId, workingDir), nil
+ return newJavaLifeCycle(pipelineId, workingDir, pipelinesFolder), nil
Review comment:
```suggestion
return newJavaLifeCycle(pipelineId, pipelinesFolder), nil
```
##########
File path: playground/backend/internal/fs_tool/fs.go
##########
@@ -51,20 +51,20 @@ type LifeCycle struct {
folderGlobs []string //folders that should be created to process code
Folder Folder
Extension Extension
- ExecutableName func(uuid.UUID, string) (string, error)
+ ExecutableName func(uuid.UUID, string, string) (string, error)
pipelineId uuid.UUID
}
// NewLifeCycle returns a corresponding LifeCycle depending on the given SDK.
// workingDir should be existed and be prepared to create/delete/modify folders into him.
-func NewLifeCycle(sdk pb.Sdk, pipelineId uuid.UUID, workingDir string) (*LifeCycle, error) {
+func NewLifeCycle(sdk pb.Sdk, pipelineId uuid.UUID, workingDir, pipelinesFolder string) (*LifeCycle, error) {
switch sdk {
case pb.Sdk_SDK_JAVA:
- return newJavaLifeCycle(pipelineId, workingDir), nil
+ return newJavaLifeCycle(pipelineId, workingDir, pipelinesFolder), nil
case pb.Sdk_SDK_GO:
- return newGoLifeCycle(pipelineId, workingDir), nil
+ return newGoLifeCycle(pipelineId, workingDir, pipelinesFolder), nil
Review comment:
```suggestion
return newGoLifeCycle(pipelineId, pipelinesFolder), nil
```
##########
File path: playground/backend/internal/fs_tool/go_fs.go
##########
@@ -25,6 +25,6 @@ const (
)
// newGoLifeCycle creates LifeCycle with go SDK environment.
-func newGoLifeCycle(pipelineId uuid.UUID, workingDir string) *LifeCycle {
- return newCompilingLifeCycle(pipelineId, workingDir, goSourceFileExtension, goExecutableFileExtension)
+func newGoLifeCycle(pipelineId uuid.UUID, workingDir, pipelinesFolder string) *LifeCycle {
+ return newCompilingLifeCycle(pipelineId, workingDir, goSourceFileExtension, goExecutableFileExtension, pipelinesFolder)
Review comment:
```suggestion
func newGoLifeCycle(pipelineId uuid.UUID, pipelinesFolder string) *LifeCycle {
return newCompilingLifeCycle(pipelineId, pipelinesFolder, goSourceFileExtension, goExecutableFileExtension)
```
##########
File path: playground/backend/internal/setup_tools/life_cycle/life_cycle_setuper.go
##########
@@ -34,14 +35,13 @@ const (
javaLogFilePlaceholder = "{logFilePath}"
goModFileName = "go.mod"
goSumFileName = "go.sum"
- baseFileFolder = "executable_files"
)
// Setup returns fs_tool.LifeCycle.
// Also, prepares files and folders needed to code processing according to sdk
-func Setup(sdk pb.Sdk, code string, pipelineId uuid.UUID, workingDir string, preparedModDir string) (*fs_tool.LifeCycle, error) {
+func Setup(sdk pb.Sdk, code string, pipelineId uuid.UUID, workingDir, pipelinesFolder, preparedModDir string) (*fs_tool.LifeCycle, error) {
Review comment:
```suggestion
func Setup(sdk pb.Sdk, code string, pipelineId uuid.UUID, pipelinesFolder, preparedModDir string) (*fs_tool.LifeCycle, error) {
```
##########
File path: playground/backend/internal/fs_tool/python_fs.go
##########
@@ -24,6 +24,6 @@ const (
)
// newPythonLifeCycle creates LifeCycle with go SDK environment.
-func newPythonLifeCycle(pipelineId uuid.UUID, workingDir string) *LifeCycle {
- return newInterpretedLifeCycle(pipelineId, workingDir, pythonExecutableFileExtension)
+func newPythonLifeCycle(pipelineId uuid.UUID, workingDir, pipelinesFolder string) *LifeCycle {
+ return newInterpretedLifeCycle(pipelineId, workingDir, pythonExecutableFileExtension, pipelinesFolder)
Review comment:
```suggestion
func newPythonLifeCycle(pipelineId uuid.UUID, pipelinesFolder string) *LifeCycle {
return newInterpretedLifeCycle(pipelineId, pipelinesFolder, pythonExecutableFileExtension)
```
##########
File path: playground/backend/internal/fs_tool/lc_constructor.go
##########
@@ -47,8 +46,8 @@ func newCompilingLifeCycle(pipelineId uuid.UUID, workingDir string, sourceFileEx
}
// newInterpretedLifeCycle creates LifeCycle for interpreted SDK environment.
-func newInterpretedLifeCycle(pipelineId uuid.UUID, workingDir string, sourceFileExtension string) *LifeCycle {
- sourceFileFolder := filepath.Join(workingDir, baseFileFolder, pipelineId.String())
+func newInterpretedLifeCycle(pipelineId uuid.UUID, workingDir, sourceFileExtension, pipelinesFolder string) *LifeCycle {
+ sourceFileFolder := filepath.Join(workingDir, pipelinesFolder, pipelineId.String())
Review comment:
```suggestion
func newInterpretedLifeCycle(pipelineId uuid.UUID, pipelinesFolder, sourceFileExtension string) *LifeCycle {
sourceFileFolder := filepath.Join(pipelinesFolder, pipelineId.String())
```
##########
File path: playground/backend/internal/setup_tools/life_cycle/life_cycle_setuper.go
##########
@@ -140,6 +140,7 @@ func updateJavaLogConfigFile(lc *fs_tool.LifeCycle) error {
}
if err = os.Rename(updatedFile.Name(), logConfigFilePath); err != nil {
+ fmt.Println(err)
Review comment:
If you want to log some error you need to use `logger`or throw the error to the previous method and log it there.
##########
File path: playground/backend/internal/fs_tool/fs.go
##########
@@ -51,20 +51,20 @@ type LifeCycle struct {
folderGlobs []string //folders that should be created to process code
Folder Folder
Extension Extension
- ExecutableName func(uuid.UUID, string) (string, error)
+ ExecutableName func(uuid.UUID, string, string) (string, error)
pipelineId uuid.UUID
}
// NewLifeCycle returns a corresponding LifeCycle depending on the given SDK.
// workingDir should be existed and be prepared to create/delete/modify folders into him.
-func NewLifeCycle(sdk pb.Sdk, pipelineId uuid.UUID, workingDir string) (*LifeCycle, error) {
+func NewLifeCycle(sdk pb.Sdk, pipelineId uuid.UUID, workingDir, pipelinesFolder string) (*LifeCycle, error) {
Review comment:
```suggestion
func NewLifeCycle(sdk pb.Sdk, pipelineId uuid.UUID, pipelinesFolder string) (*LifeCycle, error) {
```
##########
File path: playground/backend/internal/code_processing/code_processing_test.go
##########
@@ -572,7 +574,7 @@ func Test_setJavaExecutableFile(t *testing.T) {
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
- got, err := setJavaExecutableFile(tt.args.lc, tt.args.id, tt.args.service, tt.args.ctx, tt.args.executorBuilder, tt.args.dir)
+ got, err := setJavaExecutableFile(tt.args.lc, tt.args.id, tt.args.service, tt.args.ctx, tt.args.executorBuilder, tt.args.dir, tt.args.pipelinesFolder)
Review comment:
We could use the old version with `tt.args.dir`. Method `setJavaExecutableFile()` should receive one argument - directory and doesn't implement any logic like `filepath.Join(dir, pipelinesFolder)`
##########
File path: playground/backend/internal/fs_tool/fs.go
##########
@@ -51,20 +51,20 @@ type LifeCycle struct {
folderGlobs []string //folders that should be created to process code
Folder Folder
Extension Extension
- ExecutableName func(uuid.UUID, string) (string, error)
+ ExecutableName func(uuid.UUID, string, string) (string, error)
pipelineId uuid.UUID
}
// NewLifeCycle returns a corresponding LifeCycle depending on the given SDK.
// workingDir should be existed and be prepared to create/delete/modify folders into him.
-func NewLifeCycle(sdk pb.Sdk, pipelineId uuid.UUID, workingDir string) (*LifeCycle, error) {
+func NewLifeCycle(sdk pb.Sdk, pipelineId uuid.UUID, workingDir, pipelinesFolder string) (*LifeCycle, error) {
switch sdk {
case pb.Sdk_SDK_JAVA:
- return newJavaLifeCycle(pipelineId, workingDir), nil
+ return newJavaLifeCycle(pipelineId, workingDir, pipelinesFolder), nil
case pb.Sdk_SDK_GO:
- return newGoLifeCycle(pipelineId, workingDir), nil
+ return newGoLifeCycle(pipelineId, workingDir, pipelinesFolder), nil
case pb.Sdk_SDK_PYTHON:
- return newPythonLifeCycle(pipelineId, workingDir), nil
+ return newPythonLifeCycle(pipelineId, workingDir, pipelinesFolder), nil
Review comment:
```suggestion
return newPythonLifeCycle(pipelineId, pipelinesFolder), nil
```
##########
File path: playground/backend/internal/fs_tool/java_fs.go
##########
@@ -29,15 +29,15 @@ const (
)
// newJavaLifeCycle creates LifeCycle with java SDK environment.
-func newJavaLifeCycle(pipelineId uuid.UUID, workingDir string) *LifeCycle {
- javaLifeCycle := newCompilingLifeCycle(pipelineId, workingDir, JavaSourceFileExtension, javaCompiledFileExtension)
+func newJavaLifeCycle(pipelineId uuid.UUID, workingDir, pipelinesFolder string) *LifeCycle {
+ javaLifeCycle := newCompilingLifeCycle(pipelineId, workingDir, JavaSourceFileExtension, javaCompiledFileExtension, pipelinesFolder)
javaLifeCycle.ExecutableName = executableName
return javaLifeCycle
}
// executableName returns name that should be executed (HelloWorld for HelloWorld.class for java SDK)
-func executableName(pipelineId uuid.UUID, workingDir string) (string, error) {
- baseFileFolder := filepath.Join(workingDir, baseFileFolder, pipelineId.String())
+func executableName(pipelineId uuid.UUID, workingDir, pipelinesFolder string) (string, error) {
+ baseFileFolder := filepath.Join(workingDir, pipelinesFolder, pipelineId.String())
Review comment:
```suggestion
func executableName(pipelineId uuid.UUID, pipelinesFolder string) (string, error) {
baseFileFolder := filepath.Join(pipelinesFolder, pipelineId.String())
```
##########
File path: playground/backend/internal/code_processing/code_processing_test.go
##########
@@ -534,7 +534,7 @@ func TestGetLastIndex(t *testing.T) {
func Test_setJavaExecutableFile(t *testing.T) {
pipelineId := uuid.New()
- lc, _ := fs_tool.NewLifeCycle(pb.Sdk_SDK_JAVA, pipelineId, os.Getenv("APP_WORK_DIR"))
+ lc, _ := fs_tool.NewLifeCycle(pb.Sdk_SDK_JAVA, pipelineId, os.Getenv("APP_WORK_DIR"), pipelinesFolder)
Review comment:
```suggestion
lc, _ := fs_tool.NewLifeCycle(pb.Sdk_SDK_JAVA, pipelineId, filepath.Join(os.Getenv("APP_WORK_DIR"), pipelinesFolder))
```
##########
File path: playground/backend/internal/fs_tool/java_fs.go
##########
@@ -29,15 +29,15 @@ const (
)
// newJavaLifeCycle creates LifeCycle with java SDK environment.
-func newJavaLifeCycle(pipelineId uuid.UUID, workingDir string) *LifeCycle {
- javaLifeCycle := newCompilingLifeCycle(pipelineId, workingDir, JavaSourceFileExtension, javaCompiledFileExtension)
+func newJavaLifeCycle(pipelineId uuid.UUID, workingDir, pipelinesFolder string) *LifeCycle {
+ javaLifeCycle := newCompilingLifeCycle(pipelineId, workingDir, JavaSourceFileExtension, javaCompiledFileExtension, pipelinesFolder)
Review comment:
```suggestion
func newJavaLifeCycle(pipelineId uuid.UUID, pipelinesFolder string) *LifeCycle {
javaLifeCycle := newCompilingLifeCycle(pipelineId, pipelinesFolder, JavaSourceFileExtension, javaCompiledFileExtension)
```
##########
File path: playground/backend/internal/setup_tools/life_cycle/life_cycle_setuper.go
##########
@@ -34,14 +35,13 @@ const (
javaLogFilePlaceholder = "{logFilePath}"
goModFileName = "go.mod"
goSumFileName = "go.sum"
- baseFileFolder = "executable_files"
)
// Setup returns fs_tool.LifeCycle.
// Also, prepares files and folders needed to code processing according to sdk
-func Setup(sdk pb.Sdk, code string, pipelineId uuid.UUID, workingDir string, preparedModDir string) (*fs_tool.LifeCycle, error) {
+func Setup(sdk pb.Sdk, code string, pipelineId uuid.UUID, workingDir, pipelinesFolder, preparedModDir string) (*fs_tool.LifeCycle, error) {
// create file system service
- lc, err := fs_tool.NewLifeCycle(sdk, pipelineId, workingDir)
+ lc, err := fs_tool.NewLifeCycle(sdk, pipelineId, workingDir, pipelinesFolder)
Review comment:
```suggestion
lc, err := fs_tool.NewLifeCycle(sdk, pipelineId, pipelinesFolder)
```
##########
File path: playground/backend/internal/fs_tool/lc_constructor.go
##########
@@ -21,14 +21,13 @@ import (
)
const (
- baseFileFolder = "executable_files"
sourceFolderName = "src"
compiledFolderName = "bin"
)
// newCompilingLifeCycle creates LifeCycle for compiled SDK environment.
-func newCompilingLifeCycle(pipelineId uuid.UUID, workingDir string, sourceFileExtension string, compiledFileExtension string) *LifeCycle {
- baseFileFolder := filepath.Join(workingDir, baseFileFolder, pipelineId.String())
+func newCompilingLifeCycle(pipelineId uuid.UUID, workingDir, sourceFileExtension, compiledFileExtension, pipelinesFolder string) *LifeCycle {
+ baseFileFolder := filepath.Join(workingDir, pipelinesFolder, pipelineId.String())
Review comment:
```suggestion
func newCompilingLifeCycle(pipelineId uuid.UUID, pipelinesFolder, sourceFileExtension, compiledFileExtension string) *LifeCycle {
baseFileFolder := filepath.Join(pipelinesFolder, pipelineId.String())
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org