Praposal for logging support for msal-go #535

4gust · 2024-11-19T14:15:45Z

Praposal for logging support for msal-go

bgavrilMS

We discussed that we don't need the callback option for 1.18 for now.

apps/design/logPraposal.md

….18, and just focusing on the logger working on 1.21 and above * adds tests * refactors logger documentation * adds sample logger app

apps/logger/logger.go

…ion for 1.21+ * Update tests

apps/confidential/confidential_121_plus.go

apps/internal/logger/logger_121_plus.go

apps/tests/devapps/logger/logger_sample.go

apps/internal/logger/logger_121_plus.go

apps/design/logging.md

apps/confidential/confidential_121_plus.go

…g of error * Updates tests

apps/confidential/confidential.go

apps/internal/logger/logger.go

apps/internal/logger/logger_test.go

apps/confidential/confidential_121_plus.go

apps/design/logging.md

apps/internal/logger/logger.go

bgavrilMS

LGTM if this works. Not signing off as there is some work to be done.

* Some comments resolved from PR

chlowell

Are you adding tests for confidential.Client as well i.e., testing that confidential.New(WithLogger(...)) constructs a Client that writes logs?

Is the same API coming for public.Client in another PR?

apps/confidential/confidential_121_plus.go

apps/internal/slog/logger.go

apps/internal/slog/logger_121_below.go

apps/internal/slog/logger_test.go

AndyOHart

PR was created by Nilesh but it also seems good to me from working on it

… 4gust/logging-praposal

chlowell · 2025-01-17T19:28:18Z

How will we handle logging PII? Another argument to WithLogger, or could we use slog to mark a message or part of a message as PII and leave handling that up to the application?

* Adds tests * Removes from confidential client

apps/internal/slog/logger.go

apps/managedidentity/managedidentity_test.go

apps/internal/slog/logger_121_below.go

apps/managedidentity/managedidentity_with_logger_test.go

* Remove bool from WithPiiLogging

apps/managedidentity/managedidentity_with_funcs.go

… 4gust/logging-praposal

apps/managedidentity/managedidentity_with_funcs.go

apps/internal/slog/nop_handler.go

apps/internal/slog/logger_test.go

apps/managedidentity/managedidentity_with_funcs.go

apps/managedidentity/managedidentity.go

apps/managedidentity/managedidentity_with_logger_test.go

apps/managedidentity/managedidentity.go

sonarqubecloud · 2025-02-13T17:15:57Z

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

Added MD file

a58420b

4gust requested review from bgavrilMS and rayluo as code owners November 19, 2024 14:15

Updated callback support in 1.21 as well

57f484f

bgavrilMS approved these changes Nov 22, 2024

View reviewed changes

4gust changed the title ~~Added MD file~~ Praposal for logging support for msal-go Nov 25, 2024

Updated logging with files

deb5d6e

bgavrilMS reviewed Dec 9, 2024

View reviewed changes

apps/design/logPraposal.md Outdated Show resolved Hide resolved

* Pushes changes needed for logging, including removing support for 1…

e855af1

….18, and just focusing on the logger working on 1.21 and above * adds tests * refactors logger documentation * adds sample logger app

bgavrilMS reviewed Dec 19, 2024

View reviewed changes

apps/logger/logger.go Outdated Show resolved Hide resolved

bgavrilMS reviewed Dec 19, 2024

View reviewed changes

apps/logger/logger.go Outdated Show resolved Hide resolved

AndyOHart added 4 commits December 23, 2024 15:27

* Changes how logger works so that we can create a logger implementat…

0ba0540

…ion for 1.21+ * Update tests

* remove unneeded log

77d735e

* moves context.Background to internal implementation

e6aa078

* Makes changes so WithLogger is only accessible for 1.21 and above

dffa979

chlowell requested changes Jan 7, 2025

View reviewed changes

AndyOHart added 4 commits January 7, 2025 16:00

* Updates logic to create default logger instance and remove returnin…

aa59b0f

…g of error * Updates tests

* Updates to remove LoggerInterface

da84180

* Adds missing functions to help compile go1.18

435bbb6

* Updates logger to remove LoggerInterface

1a2ccbf

chlowell reviewed Jan 8, 2025

View reviewed changes

bgavrilMS reviewed Jan 9, 2025

View reviewed changes

apps/confidential/confidential_121_plus.go Outdated Show resolved Hide resolved

bgavrilMS reviewed Jan 9, 2025

View reviewed changes

apps/design/logging.md Outdated Show resolved Hide resolved

bgavrilMS reviewed Jan 9, 2025

View reviewed changes

apps/internal/logger/logger.go Outdated Show resolved Hide resolved

bgavrilMS reviewed Jan 9, 2025

View reviewed changes

AndyOHart added 2 commits January 9, 2025 19:44

* Updates packages to use slog instead of logger

bc17181

* Some comments resolved from PR

* Updates to use pointer

f4639c5

chlowell reviewed Jan 9, 2025

View reviewed changes

AndyOHart added 2 commits January 9, 2025 22:48

* update documentation

fca7a8a

* Removed some uneeded fileds and information

61b5358

AndyOHart approved these changes Jan 10, 2025

View reviewed changes

AndyOHart added 2 commits January 13, 2025 11:04

* More PR Changes

c9b47f5

Changes to how logger works

6d69a31

AndyOHart changed the base branch from main to andyohart/managed-identity January 17, 2025 13:17

Merge remote-tracking branch 'origin/andyohart/managed-identity' into…

ca09100

… 4gust/logging-praposal

AndyOHart added 4 commits January 20, 2025 14:09

* Adds logger to managed identity

41d9c77

* Adds tests * Removes from confidential client

* Some small fixes such as extra lines and some comments being removed

281d376

Update documentation WithLogger

0ef46fe

Documentation update

7232018

chlowell reviewed Jan 24, 2025

View reviewed changes

AndyOHart added 4 commits January 26, 2025 23:38

Address PR comments

dee0a5d

* Add ability to pass WithPiiLogging

2f88c40

* Update documentation

21ab563

* Update documentation

6a8d621

* Remove bool from WithPiiLogging

bgavrilMS reviewed Jan 27, 2025

View reviewed changes

apps/managedidentity/managedidentity_with_funcs.go Outdated Show resolved Hide resolved

AndyOHart added 2 commits January 28, 2025 15:19

* Removes separate WithPiiLogging to add bool to WithLogger

f284171

Merge remote-tracking branch 'origin/andyohart/managed-identity' into…

b37530d

… 4gust/logging-praposal

chlowell reviewed Jan 31, 2025

View reviewed changes

AndyOHart and others added 2 commits February 4, 2025 22:48

* PR Changes

2da41d2

Updated the file name

72611ca

chlowell reviewed Feb 7, 2025

View reviewed changes

apps/managedidentity/managedidentity_with_logger_test.go Show resolved Hide resolved

apps/managedidentity/managedidentity.go Outdated Show resolved Hide resolved

AndyOHart added 2 commits February 12, 2025 15:38

* Address some PR Comments

0122e72

* Use context of the function rather than the logOnce

6d52633

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Praposal for logging support for msal-go #535

Praposal for logging support for msal-go #535

4gust commented Nov 19, 2024

bgavrilMS left a comment

bgavrilMS left a comment

chlowell left a comment

AndyOHart left a comment

chlowell commented Jan 17, 2025

sonarqubecloud bot commented Feb 13, 2025

Praposal for logging support for msal-go #535

Are you sure you want to change the base?

Praposal for logging support for msal-go #535

Conversation

4gust commented Nov 19, 2024

bgavrilMS left a comment

Choose a reason for hiding this comment

bgavrilMS left a comment

Choose a reason for hiding this comment

chlowell left a comment

Choose a reason for hiding this comment

AndyOHart left a comment

Choose a reason for hiding this comment

chlowell commented Jan 17, 2025

sonarqubecloud bot commented Feb 13, 2025

Quality Gate passed