diff --git a/CHANGELOG.md b/CHANGELOG.md index b71460a..04edd37 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,30 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- Address the post-write-phase nice-to-have bundle from #168: + - `kaswrite.Call` now prefixes the module label onto the wrapped + `ErrUnexpectedReturnString` message, so a regression in e.g. + `mailforward.Client.Add` reads `kaswrite: unexpected ReturnString + (want TRUE): mailforward add_mailforward got "…"` instead of + losing the module hint to the canonical sentinel prefix. The + sentinel identity is unchanged — `errors.Is(err, + mailforward.ErrUnexpectedReturnString)` keeps working. + - `kasapi-cli mail forwards add --target` help no longer claims to + "replace the full target list" (which is true for `update`, not + for `add`); the add-side help now reads "repeatable; at least + one required". + - `kasapi-cli mail lists update --active` now takes an explicit + `Y|N` argument (string flag), matching the `mail accounts + update --active` surface and removing the slightly non-obvious + `--active=false` form that previously meant "deactivate". + - `runWriteE` resolves the credentials exactly once per write + invocation: the post-gate dispatch now reuses the creds + `runWriteE` already resolved for the audit login, via the new + unexported `buildAPIClientFromCreds` helper. Previously the + config + env + flag resolution ran twice — once for the audit, + once inside `BuildAPIClient` — for every real write call. + `BuildAPIClient` (the read-side seam) is unchanged. + - Cross-module convention alignment, post-#122-review parity sweep: - `account.AccountList` / `mailaccount.MailAccountList` now render the `used_*_space` column with a `" MB"` suffix as part of the diff --git a/docs/cli/kasapi-cli_mail_forwards_add.md b/docs/cli/kasapi-cli_mail_forwards_add.md index 42cfe4f..d6be65d 100644 --- a/docs/cli/kasapi-cli_mail_forwards_add.md +++ b/docs/cli/kasapi-cli_mail_forwards_add.md @@ -10,7 +10,7 @@ kasapi-cli mail forwards add
--target [--target ...] [fla ``` -h, --help help for add - --target stringArray forward target address (repeatable; replaces the full target list) + --target stringArray forward target address (repeatable; at least one required) ``` ### Options inherited from parent commands diff --git a/docs/cli/kasapi-cli_mail_lists_update.md b/docs/cli/kasapi-cli_mail_lists_update.md index 7525cb2..ed779e3 100644 --- a/docs/cli/kasapi-cli_mail_lists_update.md +++ b/docs/cli/kasapi-cli_mail_lists_update.md @@ -3,13 +3,13 @@ Replace mutable fields of a mailing list (update_mailinglist) ``` -kasapi-cli mail lists update [--subscriber ...] [--restrict-post ...] [--config-file ] [--active] [flags] +kasapi-cli mail lists update [--subscriber ...] [--restrict-post ...] [--config-file ] [--active Y|N] [flags] ``` ### Options ``` - --active activate the list; pass --active=false to deactivate it + --active string list status (Y|N) --config-file string path to the complete list configuration file (replaces the config wholesale) -h, --help help for update --restrict-post stringArray restrict-post address (repeatable; replaces the full restrict-post list) diff --git a/internal/cli/mail.go b/internal/cli/mail.go index cc567e9..ba5fcb8 100644 --- a/internal/cli/mail.go +++ b/internal/cli/mail.go @@ -84,10 +84,9 @@ func newMailListsAddCmd(opts *RootOptions) *cobra.Command { func newMailListsUpdateCmd(opts *RootOptions) *cobra.Command { var subscriber, restrictPost []string - var configFile string - var active bool + var configFile, active string cmd := &cobra.Command{ - Use: "update [--subscriber ...] [--restrict-post ...] [--config-file ] [--active]", + Use: "update [--subscriber ...] [--restrict-post ...] [--config-file ] [--active Y|N]", Short: "Replace mutable fields of a mailing list (update_mailinglist)", Args: cobra.ExactArgs(1), } @@ -113,11 +112,7 @@ func newMailListsUpdateCmd(opts *RootOptions) *cobra.Command { fields[mailinglist.FieldConfig] = string(b) } if cmd.Flags().Changed("active") { - if active { - fields[mailinglist.FieldIsActive] = "Y" - } else { - fields[mailinglist.FieldIsActive] = "N" - } + fields[mailinglist.FieldIsActive] = active } if len(fields) == 0 { return writeSpec{}, fmt.Errorf("at least one of --subscriber/--restrict-post/--config-file/--active is required") @@ -138,7 +133,7 @@ func newMailListsUpdateCmd(opts *RootOptions) *cobra.Command { cmd.Flags().StringArrayVar(&subscriber, "subscriber", nil, "list subscriber address (repeatable; replaces the full subscriber list)") cmd.Flags().StringArrayVar(&restrictPost, "restrict-post", nil, "restrict-post address (repeatable; replaces the full restrict-post list)") cmd.Flags().StringVar(&configFile, "config-file", "", "path to the complete list configuration file (replaces the config wholesale)") - cmd.Flags().BoolVar(&active, "active", false, "activate the list; pass --active=false to deactivate it") + cmd.Flags().StringVar(&active, "active", "", "list status (Y|N)") return cmd } @@ -639,7 +634,7 @@ func newMailForwardsAddCmd(opts *RootOptions) *cobra.Command { }, nil }), } - cmd.Flags().StringArrayVar(&targets, "target", nil, "forward target address (repeatable; replaces the full target list)") + cmd.Flags().StringArrayVar(&targets, "target", nil, "forward target address (repeatable; at least one required)") return cmd } diff --git a/internal/cli/mail_test.go b/internal/cli/mail_test.go index 0d53d04..4ee928a 100644 --- a/internal/cli/mail_test.go +++ b/internal/cli/mail_test.go @@ -542,7 +542,7 @@ func TestMailListsDestructiveRefuseNonTTY(t *testing.T) { t.Parallel() for _, args := range [][]string{ {"mail", "lists", "delete", "announce-example-com"}, - {"mail", "lists", "update", "announce-example-com", "--active"}, + {"mail", "lists", "update", "announce-example-com", "--active", "Y"}, } { t.Run(args[2], func(t *testing.T) { t.Parallel() @@ -580,14 +580,14 @@ func TestMailListsUpdateDryRunFieldAssembly(t *testing.T) { absent []string }{ { - "active true", - []string{"mail", "lists", "update", "L", "--active"}, + "active Y", + []string{"mail", "lists", "update", "L", "--active", "Y"}, map[string]string{"mailinglist_name": "L", "is_active": "Y"}, []string{"subscriber", "restrict_post", "config"}, }, { - "active false", - []string{"mail", "lists", "update", "L", "--active=false"}, + "active N", + []string{"mail", "lists", "update", "L", "--active", "N"}, map[string]string{"mailinglist_name": "L", "is_active": "N"}, nil, }, diff --git a/internal/cli/run.go b/internal/cli/run.go index 4c537b2..4113e35 100644 --- a/internal/cli/run.go +++ b/internal/cli/run.go @@ -97,6 +97,12 @@ type writeSpec struct { // and the dry-run audit (#131) — and only then builds the API client // and dispatches. After dispatch it always emits the success/failure // audit record via OutcomeFor, then prints the result line on success. +// +// Credentials are resolved exactly once per invocation: the resolved +// login feeds the dry-run / destructive-gate path directly, and the +// post-gate dispatch reuses the same creds via the unexported +// buildAPIClientFromCreds helper instead of re-running the +// config+env+flag resolution that BuildAPIClient would do internally. func runWriteE(opts *RootOptions, build func(args []string) (writeSpec, error)) func(*cobra.Command, []string) error { return func(cmd *cobra.Command, args []string) error { spec, err := build(args) @@ -133,7 +139,7 @@ func runWriteE(opts *RootOptions, build func(args []string) (writeSpec, error)) return err } - c, err := BuildAPIClient(opts) + c, err := buildAPIClientFromCreds(opts, creds) if err != nil { return err } diff --git a/internal/cli/wire.go b/internal/cli/wire.go index 842cca2..607aab1 100644 --- a/internal/cli/wire.go +++ b/internal/cli/wire.go @@ -36,13 +36,20 @@ func BuildAPIClient(opts *RootOptions) (*api.Client, error) { if opts == nil { return nil, UserError(errors.New("nil RootOptions"), "cli") } - - logger := buildLogger(opts.Verbose) - creds, err := resolveCreds(opts) if err != nil { return nil, err } + return buildAPIClientFromCreds(opts, creds) +} + +// buildAPIClientFromCreds wires the transport + token source on top of +// already-resolved credentials. Callers that need the login both for an +// audit trace and the actual KAS call (runWriteE / sessions delete) +// resolve once and call this helper to avoid re-running the +// config+env+flag resolution. +func buildAPIClientFromCreds(opts *RootOptions, creds config.Credentials) (*api.Client, error) { + logger := buildLogger(opts.Verbose) logger.Info("cli: credentials resolved", "login", creds.Login, "auth_type", creds.AuthType, "auth_data", "") diff --git a/internal/kaswrite/kaswrite.go b/internal/kaswrite/kaswrite.go index 8873856..e04906f 100644 --- a/internal/kaswrite/kaswrite.go +++ b/internal/kaswrite/kaswrite.go @@ -38,7 +38,10 @@ var ErrUnexpectedReturnString = errors.New("kaswrite: unexpected ReturnString (w // with label and action; a non-fault response whose ReturnString is // not "TRUE" wraps ErrUnexpectedReturnString so a future API drift // fails the mapping test instead of silently passing. label is the -// domain module name used only for the nil-response diagnostic. +// domain module name (e.g. "mailforward") prefixed onto both the +// nil-response diagnostic and the ReturnString-wrap message so log +// readers see which module surfaced the failure even after the +// canonical "kaswrite:" sentinel. func Call( ctx context.Context, caller Caller, label, action string, params map[string]any, ) (*soap.Response, error) { @@ -50,7 +53,7 @@ func Call( return nil, fmt.Errorf("%s: %s: nil response without error from Caller", label, action) } if got := resp.Body.ReturnString; got != "TRUE" { - return nil, fmt.Errorf("%w: %s got %q", ErrUnexpectedReturnString, action, got) + return nil, fmt.Errorf("%w: %s %s got %q", ErrUnexpectedReturnString, label, action, got) } return resp, nil } diff --git a/internal/kaswrite/kaswrite_test.go b/internal/kaswrite/kaswrite_test.go index 1c2e510..62775e2 100644 --- a/internal/kaswrite/kaswrite_test.go +++ b/internal/kaswrite/kaswrite_test.go @@ -53,6 +53,9 @@ func TestCallRejectsNonTrueReturnString(t *testing.T) { if !strings.Contains(err.Error(), "update_mailinglist") || !strings.Contains(err.Error(), "FALSE") { t.Errorf("err = %q, want it to contain the action and the observed value", err) } + if !strings.Contains(err.Error(), "mailinglist") { + t.Errorf("err = %q, want it to carry the module label prefix", err) + } } func TestCallPropagatesCallerError(t *testing.T) {