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 +19 -7
1 changed files with 19 additions and 7 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit c718f0d01b - Show all commits

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

@ -21,6 +21,14 @@ import (
"projects.blender.org/studio/flamenco/pkg/api"
)
var (
ErrSetupConfigUnusableSource = errors.New("sources should not have the 'is_usable' field set to false")
ErrSetupConfigEmptyStorageLocation = errors.New("'storageLocation' field must not be empty")
ErrSetupConfigEmptyPath = errors.New("'path' field must not be empty while using the 'file_association' source")
ErrSetupConfigEmptyPathOrInput = errors.New("'path' or 'input' fields must not be empty while using the 'input_path' or 'path_envvar' sources")
ErrSetupConfigEmptySource = errors.New("'source' field must not be empty")
)
func (f *Flamenco) GetVersion(e echo.Context) error {
return e.JSON(http.StatusOK, api.FlamencoVersion{
Version: appinfo.ExtendedVersion(),
@ -266,8 +274,8 @@ func (f *Flamenco) SaveSetupAssistantConfig(e echo.Context) error {
logger = logger.With().Interface("config", setupAssistantCfg).Logger()
if err := checkSetupAssistantConfig(setupAssistantCfg); err != nil {
logger.Warn().Msg("setup assistant: configuration is invalid or incomplete, " + err.Error())
return sendAPIError(e, http.StatusBadRequest, "configuration is invalid or incomplete")
logger.Error().AnErr("cause", err).Msg("setup assistant: configuration is incomplete")
return sendAPIError(e, http.StatusBadRequest, "configuration is incomplete: %v", err)
}
conf := f.config.Get()
@ -337,11 +345,11 @@ func commandNeedsQuoting(cmd string) bool {
func checkSetupAssistantConfig(config api.SetupAssistantConfig) error {
if config.StorageLocation == "" {
return errors.New("'storageLocation' field must not be empty")
return ErrSetupConfigEmptyStorageLocation
}
if !config.BlenderExecutable.IsUsable {
return errors.New("sources cannot have the 'is_usable' field set to false")
return ErrSetupConfigUnusableSource
}
switch config.BlenderExecutable.Source {
@ -350,16 +358,20 @@ func checkSetupAssistantConfig(config api.SetupAssistantConfig) error {
case api.BlenderPathSourceFileAssociation:
if config.BlenderExecutable.Path == "" {
return errors.New("'path' field must not be empty while using the 'file_association' source")
return ErrSetupConfigEmptyPath
abelli marked this conversation as resolved Outdated
Sybren A. Stüvel commented 2024-07-11 10:40:54 +02:00
Outdated
Review
Copy Link

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.
}
abelli marked this conversation as resolved Outdated
Sybren A. Stüvel commented 2024-07-05 08:32:08 +02:00
Outdated
Review
Copy Link

This function could return an error instead of a boolean. That error can then not only be used to communicate what is still missing, but also cover this case. That'll avoid the need to pass a logger to the function.

This function could return an error instead of a boolean. That error can then not only be used to communicate what is still missing, but also cover this case. That'll avoid the need to pass a logger to the function.
Sybren A. Stüvel commented 2024-07-11 10:35:54 +02:00
Outdated
Review
Copy Link

Unfortunately Gitea does not support multiple comments for the same source line. So I'll reply to the previous one, even though at this line number there is now different code.

		return errors.New("unknown 'source' field value: " + string(config.BlenderExecutable.Source))

This should use fmt.Errorf() instead.

Unfortunately Gitea does not support multiple comments for the same source line. So I'll reply to the previous one, even though at this line number there is now different code. ``` return errors.New("unknown 'source' field value: " + string(config.BlenderExecutable.Source)) ``` This should use `fmt.Errorf()` instead.
case api.BlenderPathSourceInputPath, api.BlenderPathSourcePathEnvvar:
if config.BlenderExecutable.Path == "" ||
config.BlenderExecutable.Input == "" {
return errors.New("'path' or 'input' fields must not be empty while using the 'input_path' or 'path_envvar' sources")
return ErrSetupConfigEmptyPathOrInput
}
case "":
return ErrSetupConfigEmptySource
default:
return errors.New("unknown 'source' field value: " + string(config.BlenderExecutable.Source))
return fmt.Errorf("unknown 'source' field value: %v", config.BlenderExecutable.Source)
}
return nil