feat: add trace for executables #744

wangxiaoxuan273 · 2023-07-18T10:48:09Z

Resolves #662

Signed-off-by: wangxiaoxuan273 <wangxiaoxuan119@gmail.com>

ghost · 2023-07-19T12:55:51Z

internal/cmd/options.go


+	"github.com/notaryproject/notation-go/log"
 	"github.com/notaryproject/notation/internal/trace"
+	executableTrace "github.com/oras-project/oras-credentials-go/trace"


When renaming an imported package, the local name should follow the same guidelines as package names (lower case, no under_scores or mixedCaps). (https://go.dev/blog/package-names)

Suggested change

executableTrace "github.com/oras-project/oras-credentials-go/trace"

executabletrace "github.com/oras-project/oras-credentials-go/trace"

How about calling it credentialstrace?

Changed the package name to credentialstrace.

ghost · 2023-07-19T12:59:59Z

internal/cmd/options.go

+			logger.Info("started executing credential helper program %s with action %s", executableName, action)
+		},
+		ExecuteDone: func(executableName, action string, err error) {
+			logger.Info("finished executing credential helper program %s with action %s and erro %v", executableName, action, err)


Suggested change

logger.Info("finished executing credential helper program %s with action %s and erro %v", executableName, action, err)

logger.Info("finished executing credential helper program %s with action %s and error %w", executableName, action, err)

changed accordingly.

internal/cmd/options.go

JeyJeyGao · 2023-07-19T13:21:39Z

internal/cmd/options.go

+	logger := log.GetLogger(ctx)
+	ctx = executableTrace.WithExecutableTrace(ctx, &executableTrace.ExecutableTrace{
+		ExecuteStart: func(executableName, action string) {
+			logger.Info("started executing credential helper program %s with action %s", executableName, action)


Should it be Info level or debug level? The process for accessing credential information is too detailed for a Notation user, and it is not the main logic of Notation.

changed to debug level.

internal/cmd/options.go

Signed-off-by: wangxiaoxuan273 <wangxiaoxuan119@gmail.com>

codecov-commenter · 2023-07-20T06:18:57Z

Codecov Report

Merging #744 (0e8c8d2) into main (354e74f) will increase coverage by 0.32%.
The diff coverage is 90.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##             main     #744      +/-   ##
==========================================
+ Coverage   63.66%   63.98%   +0.32%     
==========================================
  Files          40       40              
  Lines        2232     2252      +20     
==========================================
+ Hits         1421     1441      +20     
+ Misses        690      689       -1     
- Partials      121      122       +1

Impacted Files	Coverage Δ
cmd/notation/cert/show.go	`31.66% <0.00%> (ø)`
cmd/notation/inspect.go	`68.63% <ø> (ø)`
cmd/notation/list.go	`72.82% <ø> (ø)`
cmd/notation/login.go	`25.89% <ø> (ø)`
cmd/notation/key.go	`58.33% <66.66%> (ø)`
cmd/notation/cert/list.go	`52.43% <100.00%> (ø)`
cmd/notation/logout.go	`50.00% <100.00%> (ø)`
cmd/notation/sign.go	`79.38% <100.00%> (ø)`
cmd/notation/verify.go	`85.71% <100.00%> (ø)`
internal/cmd/options.go	`73.52% <100.00%> (-26.48%)`	⬇️

... and 28 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Wwwsylvia · 2023-07-20T06:44:09Z

internal/cmd/options.go


+	"github.com/notaryproject/notation-go/log"
 	"github.com/notaryproject/notation/internal/trace"
+	executabletrace "github.com/oras-project/oras-credentials-go/trace"


I think the name executabletrace looks like it traces any kind of executables, while it just traces executables for credentials usage.

changed the package name to credentialstrace.

Signed-off-by: wangxiaoxuan273 <wangxiaoxuan119@gmail.com>

Wwwsylvia · 2023-07-20T07:34:18Z

go.sum

+github.com/oras-project/oras-credentials-go v0.2.1-0.20230714083315-2afb422fe225 h1:0T0CYijlSdn0ariTr48cRn6YDl445VZf3yqCqJKksqo=
+github.com/oras-project/oras-credentials-go v0.2.1-0.20230714083315-2afb422fe225/go.mod h1:fFCebDQo0Do+gnM96uV9YUnRay0pwuRQupypvofsp4s=


Cleanup is needed.

Ran go mod tidy.

internal/cmd/options.go

Wwwsylvia · 2023-07-20T07:52:38Z

internal/cmd/options.go

 }

 // SetLoggerLevel sets up the logger based on common options.
 func (opts *LoggingFlagOpts) SetLoggerLevel(ctx context.Context) context.Context {


A question: Do we need to rename this function given that it now does more than setting logger level? 🤔 @JeyJeyGao

Good point!

Renamed the function to InitializaLogger.

Signed-off-by: wangxiaoxuan273 <wangxiaoxuan119@gmail.com>

ghost

LGTM

JeyJeyGao

LGTM

Wwwsylvia

LGTM, but I'm not a maintainer.

shizhMSFT

LGTM

priteshbandi

LGTM

priteshbandi · 2023-07-21T05:20:46Z

internal/cmd/options.go

+			if err != nil {
+				logger.Errorf("finished executing credential helper program %s with action %s and got error %w", executableName, action, err)
+			} else {
+				logger.Debugf("finished executing credential helper program %s with action %s", executableName, action)


Suggested change

logger.Debugf("finished executing credential helper program %s with action %s", executableName, action)

logger.Debugf("successfully finished executing credential helper program %s with action %s", executableName, action)

changed accordingly

Signed-off-by: wangxiaoxuan273 <wangxiaoxuan119@gmail.com>

priteshbandi

LGTM

ghost

LGTM

shizhMSFT

LGTM

Resolves notaryproject#662 --------- Signed-off-by: wangxiaoxuan273 <wangxiaoxuan119@gmail.com>

wangxiaoxuan273 requested review from JeyJeyGao, NiazFK, SteveLasker, gokarnm, justincormack, priteshbandi, rgnote and shizhMSFT as code owners July 18, 2023 10:48

wangxiaoxuan273 requested a review from a user July 18, 2023 10:48

wangxiaoxuan273 closed this Jul 19, 2023

feat: enable trace

56f56dd

Signed-off-by: wangxiaoxuan273 <wangxiaoxuan119@gmail.com>

wangxiaoxuan273 reopened this Jul 19, 2023

ghost reviewed Jul 19, 2023

View reviewed changes

JeyJeyGao reviewed Jul 19, 2023

View reviewed changes

internal/cmd/options.go Show resolved Hide resolved

wangxiaoxuan273 added 2 commits July 20, 2023 14:08

resolved comments

15a1efa

Signed-off-by: wangxiaoxuan273 <wangxiaoxuan119@gmail.com>

fixed a bug

03565ec

Signed-off-by: wangxiaoxuan273 <wangxiaoxuan119@gmail.com>

Wwwsylvia reviewed Jul 20, 2023

View reviewed changes

wangxiaoxuan273 added 2 commits July 20, 2023 14:49

updated oras-credentials-go version

0220eb3

Signed-off-by: wangxiaoxuan273 <wangxiaoxuan119@gmail.com>

renamed package

e8fe38a

Signed-off-by: wangxiaoxuan273 <wangxiaoxuan119@gmail.com>

Wwwsylvia reviewed Jul 20, 2023

View reviewed changes

JeyJeyGao reviewed Jul 20, 2023

View reviewed changes

internal/cmd/options.go Show resolved Hide resolved

Wwwsylvia reviewed Jul 20, 2023

View reviewed changes

resolved comments

32fcdaa

Signed-off-by: wangxiaoxuan273 <wangxiaoxuan119@gmail.com>

ghost previously approved these changes Jul 20, 2023

View reviewed changes

JeyJeyGao previously approved these changes Jul 20, 2023

View reviewed changes

Wwwsylvia approved these changes Jul 20, 2023

View reviewed changes

shizhMSFT previously approved these changes Jul 20, 2023

View reviewed changes

priteshbandi previously approved these changes Jul 21, 2023

View reviewed changes

priteshbandi reviewed Jul 21, 2023

View reviewed changes

added an adverb

0e8c8d2

Signed-off-by: wangxiaoxuan273 <wangxiaoxuan119@gmail.com>

wangxiaoxuan273 dismissed stale reviews from priteshbandi, shizhMSFT, JeyJeyGao, and ghost via 0e8c8d2 July 21, 2023 06:14

priteshbandi approved these changes Jul 21, 2023

View reviewed changes

ghost approved these changes Jul 21, 2023

View reviewed changes

shizhMSFT approved these changes Jul 21, 2023

View reviewed changes

shizhMSFT merged commit 161c972 into notaryproject:main Jul 21, 2023

shizhMSFT mentioned this pull request Jul 31, 2023

bump: tag and release v1.0.0 #748

Merged

6 tasks

7h3-3mp7y-m4n pushed a commit to 7h3-3mp7y-m4n/notation that referenced this pull request Mar 29, 2025

feat: add trace for executables (notaryproject#744)

ccf823a

Resolves notaryproject#662 --------- Signed-off-by: wangxiaoxuan273 <wangxiaoxuan119@gmail.com>

FeynmanZhou pushed a commit to FeynmanZhou/notation that referenced this pull request May 15, 2025

feat: add trace for executables (notaryproject#744)

6c6f4d2

Resolves notaryproject#662 --------- Signed-off-by: wangxiaoxuan273 <wangxiaoxuan119@gmail.com>

	executableTrace "github.com/oras-project/oras-credentials-go/trace"
	executabletrace "github.com/oras-project/oras-credentials-go/trace"

	logger.Info("finished executing credential helper program %s with action %s and erro %v", executableName, action, err)
	logger.Info("finished executing credential helper program %s with action %s and error %w", executableName, action, err)

		github.com/oras-project/oras-credentials-go v0.2.1-0.20230714083315-2afb422fe225 h1:0T0CYijlSdn0ariTr48cRn6YDl445VZf3yqCqJKksqo=
		github.com/oras-project/oras-credentials-go v0.2.1-0.20230714083315-2afb422fe225/go.mod h1:fFCebDQo0Do+gnM96uV9YUnRay0pwuRQupypvofsp4s=

	logger.Debugf("finished executing credential helper program %s with action %s", executableName, action)
	logger.Debugf("successfully finished executing credential helper program %s with action %s", executableName, action)

feat: add trace for executables #744

feat: add trace for executables #744

Uh oh!

Conversation

wangxiaoxuan273 commented Jul 18, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wangxiaoxuan273 Jul 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov-commenter commented Jul 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Wwwsylvia Jul 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

JeyJeyGao left a comment

Choose a reason for hiding this comment

Uh oh!

Wwwsylvia left a comment

Choose a reason for hiding this comment

Uh oh!

shizhMSFT left a comment

Choose a reason for hiding this comment

Uh oh!

priteshbandi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

priteshbandi left a comment

Choose a reason for hiding this comment

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

shizhMSFT left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

wangxiaoxuan273 Jul 20, 2023 •

edited

Loading

codecov-commenter commented Jul 20, 2023 •

edited

Loading

Wwwsylvia Jul 20, 2023 •

edited

Loading