Handle configOption failures gracefully #3195
Conversation
Signed-off-by: Steven Crespo <screswk@gmail.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Bug: Inconsistent Template Error Handling
templateConfigItems still propagates errors from resolveConfigItem, creating inconsistent behavior between ModeConfig and ModeGeneric. In ModeGeneric, the configOption* functions now handle resolution failures gracefully by returning empty strings/false, but in ModeConfig, the same failures cause template execution to fail. This breaks the PR's goal of gracefully handling config option failures in parity with KOTS behavior.
api/pkg/template/config.go#L46-L50
embedded-cluster/api/pkg/template/config.go
Lines 46 to 50 in 837b9c6
resolved |
api/pkg/template/config.go
Outdated
| resolved, err := e.resolveConfigItem(cfg.Spec.Groups[i].Items[j].Name) | ||
| if err != nil { | ||
| return nil, err | ||
| e.logger.Debugf("templateConfigItems: failed to resolve item %q, using empty values: %v", cfg.Spec.Groups[i].Items[j].Name, err) |
There was a problem hiding this comment.
if we were to log these at a warning level would the output show up for the end user or just in a log file? if the latter i think we should log at a higher level.
There was a problem hiding this comment.
i think you should use the field logger for this, for example:
| e.logger.Debugf("templateConfigItems: failed to resolve item %q, using empty values: %v", cfg.Spec.Groups[i].Items[j].Name, err) | |
| e.logger.WithError(err).WithField("item", cfg.Spec.Groups[i].Items[j].Name).Debug("templateConfigItems: failed to resolve item") |
There was a problem hiding this comment.
Updated the logs and changed the level to warning since it looks like we output to a file regardless of the level.
embedded-cluster/api/pkg/logger/logger.go
Line 25 in 8fe7073
api/pkg/template/config.go
Outdated
| resolved, err := e.resolveConfigItem(name) | ||
| if err != nil { | ||
| return "", fmt.Errorf("resolve config item: %w", err) | ||
| e.logger.Debugf("ConfigOption(%q) failed to resolve, returning empty string: %v", name, err) |
There was a problem hiding this comment.
| e.logger.Debugf("ConfigOption(%q) failed to resolve, returning empty string: %v", name, err) | |
| e.logger.WithError(err).WithField("item", name).Debug("ConfigOption: failed to resolve, returning empty string") |
…rs-instead-of-empty
screspod
deleted the
screspod/sc-131050/configoption-returns-errors-instead-of-empty
branch
* Handle configOption failures gracefully Signed-off-by: Steven Crespo <screswk@gmail.com> * f Signed-off-by: Steven Crespo <screswk@gmail.com> * f Signed-off-by: Steven Crespo <screswk@gmail.com> --------- Signed-off-by: Steven Crespo <screswk@gmail.com>
What this PR does / why we need it:
Updates existing logic in configOption item templating to handle failures gracefully in parity to KOTs behavior.
Which issue(s) this PR fixes:
https://app.shortcut.com/replicated/story/131050/configoption-returns-errors-instead-of-empty-strings-breaking-yaml-template-rendering-in-v3
Does this PR require a test?
Unit tests were added
Does this PR require a release note?
Does this PR require documentation?
NONE