Add 'copy-file' command. #104220

Bastien Montagne · 2023-06-07T14:38:56+02:00

Bastien Montagne commented

2023-06-07 14:38:56 +02:00

Initial implementation with some basic tests.

The API command only accept one source and destination path, which must
be absolute. If the destination already exists, it will not be
ovewritten, and an error is generated.

JS job definition code using this new command can easily handle list of
paths and/or relative paths if needed.

Initial implementation with some basic tests. The API command only accept one source and destination path, which must be absolute. If the destination already exists, it will not be ovewritten, and an error is generated. JS job definition code using this new command can easily handle list of paths and/or relative paths if needed.

Bastien Montagne added 1 commit 2023-06-07 14:38:57 +02:00

97960b1862 Add 'copy-file' command.

Initial implementation with some basic tests.

The API command only accept one source and destination path, which must
be absolute. If the destination already exists, it will not be
ovewritten, and an error is generated.

JS job definition code using this new command can easily handle list of
paths and/or relative paths if needed.

Sybren A. Stüvel requested review from Sybren A. Stüvel 2023-06-08 10:26:45 +02:00

Sybren A. Stüvel requested changes 2023-06-08 10:56:34 +02:00

Sybren A. Stüvel left a comment

Thanks for the PR! I've included a few notes on how the code could be simplified.

On Windows the unit test fails, so it's good that you wrote it because I overlooked a little bug as well ;-)
Testing is relatively easy if you have Wine installed:

GOOS=windows go test ./internal/worker -c -o internal/worker/gotest.exe
cd internal/worker
wine gotest.exe -test.run TestCmdCopyFile

This fails because it feels that C:\blablabla is not an absolute path. The issue is the use of path.IsAbs(), as the path module is for POSIX/URL-style paths only. use filepath.IsAbs() instead, as that takes care of OS-specific file paths.

Thanks for the PR! I've included a few notes on how the code could be simplified. On Windows the unit test fails, so it's good that you wrote it because I overlooked a little bug as well ;-) Testing is relatively easy if you have Wine installed: ```sh GOOS=windows go test ./internal/worker -c -o internal/worker/gotest.exe cd internal/worker wine gotest.exe -test.run TestCmdCopyFile ``` This fails because it feels that `C:\blablabla` is not an absolute path. The issue is the use of `path.IsAbs()`, as the `path` module is for POSIX/URL-style paths only. use `filepath.IsAbs()` instead, as that takes care of OS-specific file paths.

internal/worker/command_file_mgmt.go

						
				@ -83,0 +86,4 @@

				// It takes an absolute source and destination file path,

				// and copies the source file to its destination, if possible.

				// Missing directories in destination path are created as needed.

				// If the target path already exists, nothing happens. Destination will not be overwritten.

Sybren A. Stüvel commented

2023-06-08 10:27:55 +02:00

This comment implies that there also won't be any error returned, which is inconsistent with the PR description.

mont29 marked this conversation as resolved

internal/worker/command_file_mgmt.go

						
				@ -83,0 +87,4 @@

				// and copies the source file to its destination, if possible.

				// Missing directories in destination path are created as needed.

				// If the target path already exists, nothing happens. Destination will not be overwritten.

				func (ce *CommandExecutor) cmdCopyFile(ctx context.Context, logger zerolog.Logger, taskID string, cmd api.Command) error {

Sybren A. Stüvel commented

2023-06-08 10:48:30 +02:00

I think it might clean up the code a bit if the file copying logic was decoupled from the error logging. I'm thinking about something like this:

func (ce *CommandExecutor) cmdCopyFile(ctx context.Context, logger zerolog.Logger, taskID string, cmd api.Command) error {
	var src, dest string
	var ok bool

	if src, ok = cmdParameter[string](cmd, "src"); !ok || src == "" {
		logger.Warn().Interface("command", cmd).Msg("missing or empty 'src' parameter")
		return NewParameterMissingError("src", cmd)
	}

	if dest, ok = cmdParameter[string](cmd, "dest"); !ok || dest == "" {
		logger.Warn().Interface("command", cmd).Msg("missing or empty 'dest' parameter")
		return NewParameterMissingError("dest", cmd)
	}

	err, logMsg := copyDaFile(logger, cmd, src, dest)
	if err != nil {
		msg := fmt.Sprintf("%s: %s", cmd.Name, logMsg)
		if logErr := ce.listener.LogProduced(ctx, taskID, msg); logErr != nil {
			return logErr
		}
		return err
	}
	return nil
}

func copyDaFile(logger zerolog.Logger, cmd api.Command, src, dest string) (error, string) {
	if !path.IsAbs(src) {
		logger.Warn().Msg("source path is not an absolute path")
		msg := fmt.Sprintf("source path %q is not absolute, not copying anything", src)
		return NewParameterInvalidError("src", cmd, "path is not absolute"), msg
	}
	if !fileExists(src) {
		logger.Warn().Msg("source path does not exist, not copying anything")
		msg := fmt.Sprintf("source path %q does not exist, not copying anything", src)
		return NewParameterInvalidError("src", cmd, "path does not exist"), msg
	}

	if !path.IsAbs(dest) {
		logger.Warn().Msg("destination path is not an absolute path")
		msg := fmt.Sprintf("destination path %q is not absolute, not copying anything", src)
		return NewParameterInvalidError("dest", cmd, "path is not absolute"), msg
	}
	if fileExists(dest) {
		logger.Warn().Msg("destination path already exists, not copying anything")
		msg := fmt.Sprintf("destination path %q already exists, not copying anything", dest)
		return NewParameterInvalidError("dest", cmd, "path already exists"), msg
	}

	// This should be moved further up in the function, so that earlier log entries also include the src/dest
	logger = logger.With().
		Str("src", src).
		Str("dest", dest).
		Logger()

	logger.Info().Msg("copying file")
	// This function can then just return the same parameters, instead of
	// repeating the ce.listener.LogProduced() dance every time.
	return ce.fileCopyAndLog(ctx, taskID, cmd.Name, src, dest)
}

I think it might clean up the code a bit if the file copying logic was decoupled from the error logging. I'm thinking about something like this: ```go func (ce *CommandExecutor) cmdCopyFile(ctx context.Context, logger zerolog.Logger, taskID string, cmd api.Command) error { var src, dest string var ok bool if src, ok = cmdParameter[string](cmd, "src"); !ok || src == "" { logger.Warn().Interface("command", cmd).Msg("missing or empty 'src' parameter") return NewParameterMissingError("src", cmd) } if dest, ok = cmdParameter[string](cmd, "dest"); !ok || dest == "" { logger.Warn().Interface("command", cmd).Msg("missing or empty 'dest' parameter") return NewParameterMissingError("dest", cmd) } err, logMsg := copyDaFile(logger, cmd, src, dest) if err != nil { msg := fmt.Sprintf("%s: %s", cmd.Name, logMsg) if logErr := ce.listener.LogProduced(ctx, taskID, msg); logErr != nil { return logErr } return err } return nil } func copyDaFile(logger zerolog.Logger, cmd api.Command, src, dest string) (error, string) { if !path.IsAbs(src) { logger.Warn().Msg("source path is not an absolute path") msg := fmt.Sprintf("source path %q is not absolute, not copying anything", src) return NewParameterInvalidError("src", cmd, "path is not absolute"), msg } if !fileExists(src) { logger.Warn().Msg("source path does not exist, not copying anything") msg := fmt.Sprintf("source path %q does not exist, not copying anything", src) return NewParameterInvalidError("src", cmd, "path does not exist"), msg } if !path.IsAbs(dest) { logger.Warn().Msg("destination path is not an absolute path") msg := fmt.Sprintf("destination path %q is not absolute, not copying anything", src) return NewParameterInvalidError("dest", cmd, "path is not absolute"), msg } if fileExists(dest) { logger.Warn().Msg("destination path already exists, not copying anything") msg := fmt.Sprintf("destination path %q already exists, not copying anything", dest) return NewParameterInvalidError("dest", cmd, "path already exists"), msg } // This should be moved further up in the function, so that earlier log entries also include the src/dest logger = logger.With(). Str("src", src). Str("dest", dest). Logger() logger.Info().Msg("copying file") // This function can then just return the same parameters, instead of // repeating the ce.listener.LogProduced() dance every time. return ce.fileCopyAndLog(ctx, taskID, cmd.Name, src, dest) } ```

mont29 marked this conversation as resolved

internal/worker/command_file_mgmt.go Outdated

						
				@ -99,6 +161,73 @@ func (ce *CommandExecutor) moveAndLog(ctx context.Context, taskID, cmdName, src,

					return nil

				}

				func (ce *CommandExecutor) fileCopyAndLog(ctx context.Context, taskID, cmdName, src, dest string) error {

Sybren A. Stüvel commented

2023-06-08 10:49:34 +02:00

With the above refactor, I think this function would not need to be part of CommandExecutor any more, and become a standalone function to copy files.

With the above refactor, I think this function would not need to be part of `CommandExecutor` any more, and become a standalone function to copy files.

mont29 marked this conversation as resolved

internal/worker/command_file_mgmt.go Outdated

						
				@ -102,0 +187,4 @@

					}

					if !src_file_stat.Mode().IsRegular() {

						err := fmt.Errorf("Not a regular file")

Sybren A. Stüvel commented

2023-06-08 10:36:46 +02:00

Either wrap an existing error type, or declare an error variable that can be tested against. That way the calling code can distinguish this error from others. When there's no formatting done, use errors.New(...) to create the error.

And I think the error object creation can be done later, to avoid overlapping scopes:

// at global scope
var ErrRegularFileExpected = errors.New("not a regular file")

// at the construction:
msg := fmt.Sprintf("%s: invalid source file %q: %v", cmdName, src, err)
if err := ce.listener.LogProduced(ctx, taskID, msg); err != nil {
			return err
}
return fmt.Errorf("cannot copy %q: %w", ErrRegularFileExpected)

Either wrap an existing error type, or declare an error variable that can be tested against. That way the calling code can distinguish this error from others. When there's no formatting done, use `errors.New(...)` to create the error. And I think the error object creation can be done later, to avoid overlapping scopes: ```go // at global scope var ErrRegularFileExpected = errors.New("not a regular file") // at the construction: msg := fmt.Sprintf("%s: invalid source file %q: %v", cmdName, src, err) if err := ce.listener.LogProduced(ctx, taskID, msg); err != nil { return err } return fmt.Errorf("cannot copy %q: %w", ErrRegularFileExpected) ```

mont29 marked this conversation as resolved

Bastien Montagne added 1 commit 2023-06-08 12:47:14 +02:00

a22f7fb298 Address review comments: simplify error/reporting handling.

Also added a third test to check when source path is a directory instead
of a file.

Bastien Montagne requested review from Sybren A. Stüvel 2023-06-08 12:47:36 +02:00

Bastien Montagne commented

2023-06-08 12:50:30 +02:00

Thanks, indeed simplifies the error handling.

Not very happy with the naming of the functions now though, cmdCopyFile calling copyFile calling fileCopy... :/

Maybe cmdCopyFile -> checkAndCopyFile -> fileCopy ?

Thanks, indeed simplifies the error handling. Not very happy with the naming of the functions now though, `cmdCopyFile` calling `copyFile` calling `fileCopy`... :/ Maybe `cmdCopyFile` -> `checkAndCopyFile` -> `fileCopy` ?

Sybren A. Stüvel commented

2023-06-08 12:56:10 +02:00

Maybe the cmd parameter checks could be moved into cmdCopyFile, then we can get rid of the intermediate function.

Maybe the cmd parameter checks could be moved into `cmdCopyFile`, then we can get rid of the intermediate function.

Bastien Montagne commented

2023-06-08 13:00:30 +02:00

Maybe the cmd parameter checks could be moved into cmdCopyFile, then we can get rid of the intermediate function.

Good point!

> Maybe the cmd parameter checks could be moved into `cmdCopyFile`, then we can get rid of the intermediate function. Good point!

Bastien Montagne added 1 commit 2023-06-08 14:48:17 +02:00

dcde11fbf3 Extract logic to handle log and error into its own utils, remove intermediary funciton.

Since the whole logic handling logging messages and errors is the same
and relatively verbose, think it makes sense to isolate it into its own
utils.

NOTE: new `errorLogProcess` could also be used for other commands, like `cmdMoveDirectory`,
in a future cleanup.

Bastien Montagne commented

2023-06-08 14:49:01 +02:00

Since the whole logic handling logging messages and errors is the same and relatively verbose, think it makes sense to isolate it into its own utils.

NOTE: new errorLogProcess could also be used for other commands, like cmdMoveDirectory, in a future cleanup.

Since the whole logic handling logging messages and errors is the same and relatively verbose, think it makes sense to isolate it into its own utils. NOTE: new `errorLogProcess` could also be used for other commands, like `cmdMoveDirectory`, in a future cleanup.

Sybren A. Stüvel approved these changes 2023-06-08 16:16:27 +02:00

Sybren A. Stüvel left a comment

Much improved, thanks!

Sybren A. Stüvel merged commit 71f2947c4b into main

2023-06-08 16:20:45 +02:00

Sybren A. Stüvel referenced this issue from a commit

2023-06-08 16:20:46 +02:00

Add 'copy-file' command. (#104220)

Sign in to join this conversation.

No reviewers