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
Showing only changes of commit 1a200461d9 - Show all commits

View File

@ -265,14 +265,10 @@ func (f *Flamenco) SaveSetupAssistantConfig(e echo.Context) error {
logger = logger.With().Interface("config", setupAssistantCfg).Logger() logger = logger.With().Interface("config", setupAssistantCfg).Logger()
isConfigIncomplete := setupAssistantCfg.BlenderExecutable.Path == "" ||
setupAssistantCfg.BlenderExecutable.Input == ""
if setupAssistantCfg.StorageLocation == "" || if setupAssistantCfg.StorageLocation == "" ||
!setupAssistantCfg.BlenderExecutable.IsUsable || isBlenderPathCheckResultIncomplete(setupAssistantCfg.BlenderExecutable) {
abelli marked this conversation as resolved Outdated

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()`.
isConfigIncomplete && setupAssistantCfg.BlenderExecutable.Source != "default" { logger.Warn().Msg("setup assistant: configuration is invalid or incomplete, unable to accept")
abelli marked this conversation as resolved Outdated

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.
logger.Warn().Msg("setup assistant: configuration is incomplete, unable to accept") return sendAPIError(e, http.StatusBadRequest, "configuration is invalid or incomplete")
return sendAPIError(e, http.StatusBadRequest, "configuration is incomplete")
} }
conf := f.config.Get() conf := f.config.Get()
@ -339,3 +335,27 @@ func flamencoManagerDir() (string, error) {
func commandNeedsQuoting(cmd string) bool { func commandNeedsQuoting(cmd string) bool {
return strings.ContainsAny(cmd, "\n\t;()") return strings.ContainsAny(cmd, "\n\t;()")
} }
func isBlenderPathCheckResultIncomplete(checkResult api.BlenderPathCheckResult) bool {
switch checkResult.Source {
case api.BlenderPathSourceDefault:
if !checkResult.IsUsable {
return true
abelli marked this conversation as resolved Outdated

The check for IsUsable can be moved out of the switch since it's the same for every case anyway.

The check for `IsUsable` can be moved out of the `switch` since it's the same for every `case` anyway.
}
return false
case api.BlenderPathSourceFileAssociation:
if !checkResult.IsUsable ||
checkResult.Path == "" {
return true
}
return false
case api.BlenderPathSourceInputPath, api.BlenderPathSourcePathEnvvar:
if !checkResult.IsUsable ||
checkResult.Path == "" ||
checkResult.Input == "" {
return true
}
return false
}
return true
}
abelli marked this conversation as resolved Outdated

Add another case "". That way the dynamically generated error below doesn't just show "unknown 'source' field value: " if the source field is empty.

Another solution would be to use the %q format specifier in the below-suggested call to fmt.Errorf(), but that would always put quotes around the value. If that error is then subsequently logged with something like log.Error().Err(err).Msg("whatever"), those quotes will get escaped and things will be uglier than necessary. So that's why I think it's nicer to handle the empty string as a special case, and keep the non-empty-string case without quotes.

Add another `case ""`. That way the dynamically generated error below doesn't just show `"unknown 'source' field value: "` if the `source` field is empty. Another solution would be to use the `%q` format specifier in the below-suggested call to `fmt.Errorf()`, but that would _always_ put quotes around the value. If that error is then subsequently logged with something like `log.Error().Err(err).Msg("whatever")`, those quotes will get escaped and things will be uglier than necessary. So that's why I think it's nicer to handle the empty string as a special case, and keep the non-empty-string case without quotes.