Skip to content

fix: eliminate global logger side effects in mapper package#151

Open
tekenstam wants to merge 4 commits intomasterfrom
fix/validate-return-error
Open

fix: eliminate global logger side effects in mapper package#151
tekenstam wants to merge 4 commits intomasterfrom
fix/validate-return-error

Conversation

@tekenstam
Copy link
Copy Markdown
Member

@tekenstam tekenstam commented Mar 31, 2026

Summary

  • Change MapperArguments.Validate() to return error instead of calling log.Fatal(), enabling library consumers to handle validation failures gracefully (fixes Bug: Validate() calls log.Fatal() preventing library error handling #143)
  • Replace the global log.SetOutput(io.Discard) call in New() with a per-instance *log.Logger field on AuthMapper, so creating a silent mapper no longer silences logging for the entire process (fixes Bug: New() with isCommandline=false disables logging globally #144)
  • Convert WithRetry from a package-level function to an AuthMapper method so it uses the instance logger
  • Remove the unused LoggingEnabled field and the init() function that mutated global log flags
  • Update all callers (Upsert, Remove, RemoveByUsername, Get) to check and propagate validation errors and use the instance logger
  • Add unit tests for both fixes

Test plan

  • All existing unit tests pass (go test ./...)
  • New tests cover every validation branch: invalid retry count, missing role/user ARN, mutually exclusive flags, missing username, invalid format, missing map selection, and successful validation
  • New tests verify per-instance logger isolation (two AuthMapper instances with different logging settings don't interfere)
  • Verify CI passes on this PR

🤖 Generated with Claude Code

Change MapperArguments.Validate() signature from Validate() to
Validate() error so that callers can handle validation failures
gracefully. This is required for library usage where log.Fatal()
would terminate the host application.

Update all callers (Upsert, Remove, RemoveByUsername, Get) to check
and propagate the returned error. Add unit tests for all validation
paths.

Closes #143

Signed-off-by: Todd Ekenstam <todd_ekenstam@intuit.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Todd Ekenstam <todd_ekenstam@intuit.com>
@tekenstam tekenstam requested a review from a team as a code owner March 31, 2026 16:19
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Mar 31, 2026

Codecov Report

❌ Patch coverage is 78.94737% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.98%. Comparing base (5cf8555) to head (969f0c2).

Files with missing lines Patch % Lines
pkg/mapper/remove.go 63.63% 2 Missing and 2 partials ⚠️
pkg/mapper/get.go 33.33% 1 Missing and 1 partial ⚠️
pkg/mapper/upsert.go 75.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #151      +/-   ##
==========================================
+ Coverage   68.97%   69.98%   +1.01%     
==========================================
  Files          12       12              
  Lines         593      593              
==========================================
+ Hits          409      415       +6     
+ Misses        165      162       -3     
+ Partials       19       16       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

New() with isCommandline=false called log.SetOutput(io.Discard), which
silenced the global logger for the entire process. Replace the unused
LoggingEnabled field with a per-instance Logger, and convert WithRetry
to a method so it can use the instance logger. Fixes #144.

Signed-off-by: Todd Ekenstam <todd_ekenstam@intuit.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Todd Ekenstam <todd_ekenstam@intuit.com>
@tekenstam tekenstam changed the title fix: return errors from Validate() instead of calling log.Fatal() fix: eliminate global logger side effects in mapper package Mar 31, 2026
Todd Ekenstam and others added 2 commits March 31, 2026 09:58
Error-path log messages in removeAuth and removeAuthByUser duplicated
information already conveyed by the returned error. CLI callers surface
these errors via log.Fatal, so users saw the same failure twice. Remove
the redundant logs and improve the error messages to include the ARN.

Signed-off-by: Todd Ekenstam <todd_ekenstam@intuit.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Todd Ekenstam <todd_ekenstam@intuit.com>
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.

Bug: New() with isCommandline=false disables logging globally Bug: Validate() calls log.Fatal() preventing library error handling

2 participants