Skip to content

fix(garm-configurator): log full config validation error, shorten blocked status - #268

Merged
cbartz merged 2 commits into
mainfrom
fix/garm-configurator-log-config-validation
Jul 8, 2026
Merged

fix(garm-configurator): log full config validation error, shorten blocked status#268
cbartz merged 2 commits into
mainfrom
fix/garm-configurator-log-config-validation

Conversation

@cbartz

@cbartz cbartz commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

When config validation fails, garm-configurator put the entire pydantic ValidationError (often multi-line) straight into the unit status message. Juju truncates status messages to their first line, so operators only ever saw the generic header (e.g. 1 validation error for RunnerConfig) with no field/reason, and nothing reached juju debug-log.

This aligns the charm with its siblings (planner-operator, webhook-gateway-operator): the full detail is logged at warning level, and the status is a short, stable Invalid charm config. warning (not error) is used because _reconcile runs on every event, so a persistently misconfigured charm would otherwise spam error-level logs.

Why we need it

The truncated status was actively misleading — an operator hitting an invalid runner option had no way to see which option or why without dumping juju status --format=yaml. The actionable detail now lives in debug-log where operators expect it.

Checklist

  • Changes comply with the project's coding standards and guidelines (see CONTRIBUTING.md and STYLE.md)
  • CONTRIBUTING.md has been updated upon changes to the contribution/development process (e.g. changes to the way tests are run)
  • Technical author has been assigned to review the PR in case of documentation changes (usually *.md files)
  • I updated docs/changelog.md with user-relevant changes
  • I used AI to assist with preparing this PR
  • I added or updated tests as needed (unit and integration)
  • If integration test modules are used: I updated the workflow configuration
  • If this PR involves a Grafana dashboard: I added a screenshot of the dashboard
  • If this PR involves Terraform: terraform fmt passes and tflint reports no errors
  • If this PR involves Rockcraft: I updated the version
  • If this PR adds/removes a charm, or changes a charm's base class, conventions, tooling, or repo structure: I updated the relevant AGENTS.md
  • If this PR changes .copilot-collections.yaml or .github/instructions/: I re-checked the AGENTS.md "12-factor divergences" guidance

Test plan

  • tox -c tox.toml -e fmt lint static unit all pass in charms/garm-configurator (unit: 51/51).
  • Existing invalid-config unit tests (Scenario-based) were updated: they now assert the short Invalid charm config status and that the specific validation detail is emitted as a WARNING log record (via caplog). No parallel tests were added — existing ones were extended.

Review focus

  • The status message for all CharmConfigInvalidError paths (not just RunnerConfig) collapses to the single generic Invalid charm config. This is intentional, but reviewers may want to confirm that losing the per-cause status text (e.g. Missing required configuration: openstack-auth-url) is acceptable — that detail now lives only in the log. If a shorter cause-specific status is preferred, that's a larger change to how CharmConfigInvalidError.msg is structured.

Potential breaking changes / new dependencies

None. No new dependencies, APIs, or config options. Behavioral change is limited to status text + log output.

…cked status

Invalid configuration put the entire (often multi-line) pydantic
validation error into the unit status message. Juju truncates status
messages to their first line, so operators only saw the generic header
(e.g. "1 validation error for RunnerConfig") and nothing reached
juju debug-log.

Match the sibling charms (planner-operator, webhook-gateway-operator):
log the full detail at warning level and set a short, stable
"Invalid charm config" status. warning (not error) is used because
_reconcile runs on every event, so a persistently misconfigured charm
would otherwise spam error-level logs.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR improves the operator experience for the garm-configurator charm when charm config validation fails: instead of putting a multi-line Pydantic validation error into Juju status (which gets truncated), it logs the full detail at WARNING level and sets a short, stable Blocked status message.

Changes:

  • Log full CharmConfigInvalidError details at WARNING level during _reconcile.
  • Collapse Blocked unit status to a stable Invalid charm config message.
  • Update Scenario-based unit tests to assert both the short Blocked status and presence of validation detail in warning logs; add a changelog entry.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
docs/changelog.md Adds a 2026-07-08 entry describing the user-visible status/logging behavior change.
charms/garm-configurator/src/charm.py Logs config validation details at WARNING and sets a short BlockedStatus message.
charms/garm-configurator/tests/unit/test_charm.py Updates invalid-config tests to check for the stable status and warning log detail via caplog.

@cbartz
cbartz marked this pull request as ready for review July 8, 2026 11:04

@yanksyoon yanksyoon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can think about how to raise errors that puts charm in blocked status with actionable message. LGTM!

Drop the status genericization from the previous commit. Only 1 of the ~15
CharmConfigInvalidError sites produced the multi-line pydantic message that
Juju truncates to an unhelpful first line; the other 14 were already short,
actionable single lines that displayed fine in the status. Collapsing every
status to "Invalid charm config" regressed those.

Restore BlockedStatus(e.msg) and keep only the logging addition, so the
status stays actionable and the full detail is still discoverable in
debug-log. Making the one pydantic path emit a single-line message (so its
status is actionable too) is left to a follow-up.
@cbartz

cbartz commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator Author

Maybe we can think about how to raise errors that puts charm in blocked status with actionable message. LGTM!

Thanks. I changed it back to put the full msg in the status.

@cbartz
cbartz merged commit 864ec42 into main Jul 8, 2026
62 checks passed
@cbartz
cbartz deleted the fix/garm-configurator-log-config-validation branch July 8, 2026 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants