fix(sentinel): apply certificate config changes by certificate type - #297
Closed
thiagoesteves wants to merge 2 commits into
Closed
fix(sentinel): apply certificate config changes by certificate type#297thiagoesteves wants to merge 2 commits into
thiagoesteves wants to merge 2 commits into
Conversation
The config watcher pattern-matched on `details: %{domains: ...}` but
`diff_certificates` builds the details map keyed by certificate type
(:acme, :importer, ...), never :domains. The pattern never matched, so
certificate managers were never stopped or started when certificates
changed in the YAML.
Iterate over the actual certificate type keys in the details map
instead of hardcoding :domains. Stop the manager once for any removed
or modified type, and start it for each added or modified type.
Also fix the test fixture that used a non-existent `:domains` type,
masking the bug, and add apply tests that verify the stop/start calls.
Risk assessment:
- Impact: Certificate changes in deployex.yaml now correctly stop and
restart the certificate manager, as the UI has been showing they would
- Blast radius: sentinel config watcher only; certificate manager
start/stop calls are the only behavior change
- Regression risk: low - the old code never matched, so no working
behavior is replaced; the new code follows the same stop-then-start
sequence used for added/removed applications
- Rollback: plain commit revert
thiagoesteves
added a commit
that referenced
this pull request
Aug 13, 2026
The PR guidelines referenced Claude Code by name, which confused agents running on other models. Generalize the attribution line to credit Devin with the model name as a parameter (e.g. GLM-5.2 High, Claude Sonnet 4.5), so any AI agent can fill it in correctly regardless of which model it runs on. Impact: agents reading AGENTS.md now know to credit Devin and their own model rather than copying a Claude Code line that does not apply. Blast radius: only AGENTS.md. No code or config changes. Regression risk: none. Documentation-only change. Rollback: revert this commit.
thiagoesteves
added a commit
that referenced
this pull request
Aug 13, 2026
The PR guidelines referenced Claude Code by name, which confused agents running on other models. Generalize the attribution line to cover both Devin and Claude Code directly, each with the model name as a parameter (e.g. GLM-5.2 High, Claude Sonnet 4.5), so any AI agent can fill it in correctly regardless of which tool or model it runs on. Impact: agents reading AGENTS.md now know to credit the tool they are running through (Devin or Claude Code) and their own model, rather than copying a single hardcoded line that may not apply. Blast radius: only AGENTS.md. No code or config changes. Regression risk: none. Documentation-only change. Rollback: revert this commit.
thiagoesteves
added a commit
that referenced
this pull request
Aug 14, 2026
The PR guidelines referenced Claude Code by name, which confused agents running on other models. Generalize the attribution line to cover both Devin and Claude Code directly, each with the model name as a parameter (e.g. GLM-5.2 High, Claude Sonnet 4.5), so any AI agent can fill it in correctly regardless of which tool or model it runs on. Impact: agents reading AGENTS.md now know to credit the tool they are running through (Devin or Claude Code) and their own model, rather than copying a single hardcoded line that may not apply. Blast radius: only AGENTS.md. No code or config changes. Regression risk: none. Documentation-only change. Rollback: revert this commit.
Owner
Author
|
Closing this PR - after review with the maintainer, the certificate type in the YAML schema is "domains" (atom :domains), not :acme. The original watcher code correctly matched on :domains. My change was based on a wrong assumption about the certificate type and broke the correct test fixture. There is no bug in the certificate config apply path. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
details: %{domains: ...}butdiff_certificatesbuilds the details map keyed by certificate type (:acme,:importer, ...), never:domains. The pattern never matched, so certificate managers were never stopped or started when certificates changed in the YAML.:domains. Stop the manager once for any removed or modified type, and start it for each added or modified type.:domainstype, masking the bug, and add apply tests that verify the stop/start calls.Test plan
mix test apps/sentinel/test/config/ --warnings-as-errors(53 tests)mix test apps/deployex_web/test/deployex_web/live/applications/watcher_test.exs --warnings-as-errors(7 tests)mix format --check-formattedmix credo --strict apps/sentinel/lib/sentinel/config/watcher.exRisk assessment
deployex.yamlnow correctly stop and restart the certificate manager, as the UI has been showing they would.Generated with Devin powered by GLM-5.2 High