Skip to content

Conversation

@JungukCho
Copy link
Contributor

Reason for Change:

This PR is to print running NPM version in stdout which can help diagnose errors or know tested version (e.g., which version is used for testing).
In addition, this PR replace log with klog for better stdout format in npm.go and main.go

Issue Fixed:

Requirements:

Notes:

@JungukCho JungukCho changed the title [NPM] Print running NPM version in stdout which helps diagnose errors. [NPM] Print running NPM version in stdout. Jul 29, 2021
@matmerr
Copy link
Member

matmerr commented Aug 2, 2021

maybe with the cli tools we should add a version command

@vakalapa
Copy link
Contributor

vakalapa commented Aug 2, 2021

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@JungukCho
Copy link
Contributor Author

JungukCho commented Aug 2, 2021

maybe with the cli tools we should add a version command

That sounds useful.
I am not familiar with cli tools.
@matmerr Would you pick it up in a different PR or want me to add it with this PR (while I prefer former)?


npMgr := npm.NewNetworkPolicyManager(clientset, factory, exec.New(), version)
metrics.CreateTelemetryHandle(npMgr.GetAppVersion(), npm.GetAIMetadata())
metrics.CreateTelemetryHandle(version, npm.GetAIMetadata())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you fix this lint please, apart from that the PR looks good.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we agreed, it will come with another PR about metric.

Copy link
Contributor

@vakalapa vakalapa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@JungukCho JungukCho merged commit 47461d0 into Azure:master Aug 5, 2021
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.

3 participants