fix(database): post-#122 review followups - #184
Merged
Conversation
Address the Nice-to-haves the post-merge review of PR #183 surfaced: - Split databases add/update flag bindings into disjoint databaseAddFlags / databaseUpdateFlags structs, mirroring the ddnsuser slice. The flag names stay identical, but each subcommand's --help describes the flags from its own perspective (initial vs replacement, required vs optional) and cobra rejects an unknown flag at parse time. New TestDatabasesFlagSetsAreDisjoint pins the boundary structurally. - Drop omitempty from Database.in_progress so the JSON/YAML shape matches the majority of read modules (mailaccount, mailinglist, sambauser, ftpuser, account). The KAS API has returned in_progress on every captured fixture row, so the previous omitempty added drift without shielding callers from a missing key. - Introduce database.InProgressFalse / .InProgressTrue constants for the literal "FALSE" / "TRUE" strings so mapping code and tests share one source of truth; the read tests now reference these constants instead of repeating the literal. - Align the used_database_space rendering across views: the list cell now carries the " MB" unit as part of the value (matching the singular detail row), and the list header is now bare "USED" rather than the unit-bearing "USED_MB". One convention across both formats. - Replace database.Client.Add's combined "non-empty password, comment and allowed_hosts" validation with per-field errors, so a caller hitting the domain validator (rather than the CLI's per-flag checks) can tell which field broke. The matching write_test asserts the per-field error substring. - Extract databaseDeleteConfirm helper and export ConfirmAction.Summary so the louder "permanently delete" verb is pinned by a test (TestDatabasesDeleteConfirmIsLouder) rather than only a source-code comment. A subsequent refactor of either the slice or the shared ConfirmAction template cannot silently weaken the loudness contract. - Add TestDatabasesDeleteDryRunEmitsAuditLine: pins that the runWriteE seam emits the #131 audit record on stderr even on --dry-run (outcome=dry-run action=delete_database target=<login> database_login=<login>), sealing the audit-emission path for the database delete subcommand without requiring an HTTP test scaffold. Pure refactor + test add: behaviour for any previously-valid CLI invocation is unchanged.
User-flagged correction on top of the post-#122 followups: the KAS API treats an empty database_allowed_hosts as the documented "any host may connect" wildcard, not as a missing parameter. The domain-level Add validation and the CLI's --allowed-hosts required check both rejected the empty value, which was wrong. - internal/database/write.go: drop the AllowedHosts == "" check; doc on Spec records the wildcard semantics. - internal/database/write_test.go: drop the "missing allowed_hosts" validation case; add a positive test asserting Add accepts an empty AllowedHosts and dispatches the wire call verbatim. - internal/cli/databases.go: --allowed-hosts is now optional; the add command's --help (and the regenerated docs/cli/) documents that an omitted / empty value is the wildcard. - internal/cli/databases_test.go: drop the "missing --allowed-hosts" rejection case; add TestDatabasesAddOptionalAllowedHosts pinning that the empty value reaches the dry-run preview's params with database_allowed_hosts = "" (the wildcard sent verbatim on the wire). - CHANGELOG.md: amend the per-field-validation bullet to note the wildcard semantics change.
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
Addresses the Nice-to-haves the post-merge review of #122 (PR #183) surfaced for the database write slice.
omitemptyfromDatabase.in_progress; align with majority of read modules.database.InProgressFalse/database.InProgressTrueconstants.used_database_spacerendering: list cells carry the " MB" suffix (matching singular); header is bareUSED.database.Client.Add(replaces the combined message).databaseDeleteConfirmhelper + exportConfirmAction.Summary→TestDatabasesDeleteConfirmIsLouderpins the louder"permanently delete"verb.TestDatabasesDeleteDryRunEmitsAuditLineseals therunWriteE → WriteAuditglue for database delete (dry-run audit emission on stderr).Pure refactor + test add — no behavioural change for any previously-valid CLI invocation.
Cross-slice parity for the same findings (ddns omitempty, account " MB" suffix, per-field validation across cronjob/ddns/ftpuser/mailforward/mailinglist/sambauser,
docs/usage/destructive-writes.mdpage refactor) lands separately in the follow-up PR-B.