studio/flamenco
studio/flamenco
11
29
Fork 38
Code Issues 62 Pull Requests 8 Releases Wiki Activity

Manager: allow setup to finish without Blender #104306

Manually merged
Sybren A. Stüvel merged 34 commits from abelli/flamenco:issue100195 into main 2024-09-09 11:22:42 +02:00
Conversation 17 Commits 34 Files Changed 9 +27 -38
1 changed files with 27 additions and 38 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 6d745ce0b4 - Show all commits

65
internal/manager/api_impl/meta.go
View File

@ -182,12 +182,11 @@ func (f *Flamenco) FindBlenderExePath(e echo.Context) error {
logger.Warn().AnErr("cause", err).Msg("there was an issue finding Blender") logger.Warn().AnErr("cause", err).Msg("there was an issue finding Blender")
return sendAPIError(e, http.StatusInternalServerError, "there was an issue finding Blender: %v", err) return sendAPIError(e, http.StatusInternalServerError, "there was an issue finding Blender: %v", err)
default: default:
isUsable := true
response = append(response, api.BlenderPathCheckResult{ response = append(response, api.BlenderPathCheckResult{
IsUsable: &isUsable, IsUsable: true,
Input: &result.Input, Input: result.Input,
Path: &result.FoundLocation, Path: result.FoundLocation,
Cause: &result.BlenderVersion, Cause: result.BlenderVersion,
Source: result.Source, Source: result.Source,
}) })
} }
@ -201,12 +200,11 @@ func (f *Flamenco) FindBlenderExePath(e echo.Context) error {
case err != nil: case err != nil:
logger.Info().AnErr("cause", err).Msg("there was an issue finding Blender as 'blender' on $PATH") logger.Info().AnErr("cause", err).Msg("there was an issue finding Blender as 'blender' on $PATH")
default: default:
isUsable := true
response = append(response, api.BlenderPathCheckResult{ response = append(response, api.BlenderPathCheckResult{
IsUsable: &isUsable, IsUsable: true,
Input: &result.Input, Input: result.Input,
Path: &result.FoundLocation, Path: result.FoundLocation,
Cause: &result.BlenderVersion, Cause: result.BlenderVersion,
Source: result.Source, Source: result.Source,
}) })
} }
@ -225,39 +223,32 @@ func (f *Flamenco) CheckBlenderExePath(e echo.Context) error {
} }
command := toCheck.Path command := toCheck.Path
var cause string
var isUsable bool
var path string
logger = logger.With().Str("command", command).Logger() logger = logger.With().Str("command", command).Logger()
logger.Info().Msg("checking whether this command leads to Blender") logger.Info().Msg("checking whether this command leads to Blender")
ctx := e.Request().Context() ctx := e.Request().Context()
checkResult, err := find_blender.CheckBlender(ctx, command) checkResult, err := find_blender.CheckBlender(ctx, command)
response := api.BlenderPathCheckResult{ response := api.BlenderPathCheckResult{
Input: &command, Input: command,
Source: checkResult.Source, Source: checkResult.Source,
Path: &path,
IsUsable: &isUsable,
Cause: &cause,
} }
switch { switch {
case errors.Is(err, exec.ErrNotFound): case errors.Is(err, exec.ErrNotFound):
*response.Cause = "Blender could not be found" response.Cause = "Blender could not be found"
case err != nil: case err != nil:
*response.Cause = fmt.Sprintf("There was an error running the command: %v", err) response.Cause = fmt.Sprintf("There was an error running the command: %v", err)
default: default:
*response.IsUsable = true response.IsUsable = true
*response.Path = checkResult.FoundLocation response.Path = checkResult.FoundLocation
*response.Cause = fmt.Sprintf("Found %v", checkResult.BlenderVersion) response.Cause = fmt.Sprintf("Found %v", checkResult.BlenderVersion)
} }
logger.Info(). logger.Info().
Str("input", *response.Input). Str("input", response.Input).
Str("foundLocation", *response.Path). Str("foundLocation", response.Path).
Str("result", *response.Cause). Str("result", response.Cause).
Bool("isUsable", *response.IsUsable). Bool("isUsable", response.IsUsable).
Msg("result of command check") Msg("result of command check")
return e.JSON(http.StatusOK, response) return e.JSON(http.StatusOK, response)
@ -274,15 +265,13 @@ func (f *Flamenco) SaveSetupAssistantConfig(e echo.Context) error {
logger = logger.With().Interface("config", setupAssistantCfg).Logger() logger = logger.With().Interface("config", setupAssistantCfg).Logger()
isConfigInvalid := setupAssistantCfg.StorageLocation == "" || isConfigIncomplete := setupAssistantCfg.StorageLocation == "" ||
setupAssistantCfg.BlenderExecutable.IsUsable == nil !setupAssistantCfg.BlenderExecutable.IsUsable ||
abelli marked this conversation as resolved Outdated
Sybren A. Stüvel commented 2024-07-11 10:32:12 +02:00
Outdated
Review
Copy Link

The work cannot be done, and so this should be logged as an error, not a warning.

I usually log errors as a 'cause' field:

logger.Error().AnErr("cause", err).Msg("setup assistant: configuration is incomplete")

Alternatively, if you want to keep the error message as part of the log message, you could use .Msgf(...) instead:

logger.Error().Msgf("setup assistant: configuration is incomplete, %v", err)

%v is Go's generic "put the thing's value here", and should result in the value of err.Error().

The work cannot be done, and so this should be logged as an error, not a warning. I usually log errors as a 'cause' field: ```go logger.Error().AnErr("cause", err).Msg("setup assistant: configuration is incomplete") ``` Alternatively, if you want to keep the error message as part of the log message, you could use `.Msgf(...)` instead: ```go logger.Error().Msgf("setup assistant: configuration is incomplete, %v", err) ``` `%v` is Go's generic "put the thing's value here", and should result in the value of `err.Error()`.
setupAssistantCfg.BlenderExecutable.Path == ""
abelli marked this conversation as resolved Outdated
Sybren A. Stüvel commented 2024-07-11 10:32:44 +02:00
Outdated
Review
Copy Link

It's fine to include the actual error message in the response as well, so that the caller knows what's wrong without checking the Manager log.

It's fine to include the actual error message in the response as well, so that the caller knows what's wrong without checking the Manager log.
isConfigIncomplete := setupAssistantCfg.BlenderExecutable.Path == nil || if isConfigIncomplete && setupAssistantCfg.BlenderExecutable.Source != "default" {
setupAssistantCfg.BlenderExecutable.Input == nil logger.Warn().Msg("setup assistant: configuration is incomplete, unable to accept")
return sendAPIError(e, http.StatusBadRequest, "configuration is incomplete")
if isConfigInvalid || setupAssistantCfg.BlenderExecutable.Source != "default" && isConfigIncomplete {
logger.Warn().Msg("setup assistant: configuration is invalid or incomplete, unable to accept")
return sendAPIError(e, http.StatusBadRequest, "configuration is invalid or incomplete")
} }
conf := f.config.Get() conf := f.config.Get()
@ -297,10 +286,10 @@ func (f *Flamenco) SaveSetupAssistantConfig(e echo.Context) error {
case api.BlenderPathSourcePathEnvvar: case api.BlenderPathSourcePathEnvvar:
// The input command can be found on $PATH, and thus we don't need to save // The input command can be found on $PATH, and thus we don't need to save
// the absolute path to Blender here. // the absolute path to Blender here.
executable = *setupAssistantCfg.BlenderExecutable.Input executable = setupAssistantCfg.BlenderExecutable.Input
case api.BlenderPathSourceInputPath: case api.BlenderPathSourceInputPath:
// The path should be used as-is. // The path should be used as-is.
executable = *setupAssistantCfg.BlenderExecutable.Path executable = setupAssistantCfg.BlenderExecutable.Path
} }
if commandNeedsQuoting(executable) { if commandNeedsQuoting(executable) {
executable = strconv.Quote(executable) executable = strconv.Quote(executable)