Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

put back the config to write logs to the file instead of only stdout logs #2107

Merged
merged 2 commits into from
Aug 9, 2023
Merged

put back the config to write logs to the file instead of only stdout logs #2107

merged 2 commits into from
Aug 9, 2023

Conversation

paulyufan2
Copy link
Contributor

Reason for Change:

This PR is to put back thye config to write to the file instead of only stdout logs

Issue Fixed:

Requirements:

Notes:

@paulyufan2 paulyufan2 added the cni Related to CNI. label Aug 7, 2023
@paulyufan2 paulyufan2 requested a review from a team as a code owner August 7, 2023 23:58
@paulyufan2 paulyufan2 requested a review from nairashu August 7, 2023 23:58
@paulyufan2 paulyufan2 changed the title put back thye config to write to the file instead of only stdout logs put back the config to write logs to the file instead of only stdout logs Aug 7, 2023
tamilmani1989
tamilmani1989 requested changes Aug 8, 2023
View reviewed changes
@vipul-21
Copy link
Contributor

vipul-21 commented Aug 8, 2023 •
edited
Loading

@paulyufan2 make sure you run the Azure Container Networking PR - submodules pipeline. It is getting skipped as the criteria to trigger the pipeline didn't match.( Will change that in separate PR: #2108 )

@paulyufan2
Copy link
Contributor Author

paulyufan2 commented Aug 8, 2023 •
edited by jpayne3506
Loading

@paulyufan2 make sure you run the Azure Container Networking PR - submodules pipeline. It is getting skipped as the criteria to trigger the pipeline didn't match.( Will change that in separate PR: #2108 )

Pm for pipeline run.

@paulyufan2 paulyufan2 closed this Aug 8, 2023
@paulyufan2 paulyufan2 reopened this Aug 8, 2023
ramiro-gamarra
ramiro-gamarra reviewed Aug 8, 2023
View reviewed changes
cni/network/plugin/main.go
@@ -15,9 +15,10 @@ import (
"github.com/Azure/azure-container-networking/aitelemetry"
"github.com/Azure/azure-container-networking/cni"
"github.com/Azure/azure-container-networking/cni/api"
"github.com/Azure/azure-container-networking/cni/log"
zaplog "github.com/Azure/azure-container-networking/cni/log"
Copy link
Contributor

Choose a reason for hiding this comment

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

is this change needed? i'm not sure why the import alias is prefixed with zap (seems like an implementation detail). less changed lines if you just alias the new conflicting import?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi Ramiro, because we want to rename the import packet since they are all using "log." to avoid conflicts

@paulyufan2 paulyufan2 enabled auto-merge (squash) August 9, 2023 02:30
@paulyufan2 paulyufan2 merged commit e9ddb87 into master Aug 9, 2023
@paulyufan2 paulyufan2 deleted the zaplog branch August 9, 2023 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ramiro-gamarra ramiro-gamarra ramiro-gamarra left review comments

@tamilmani1989 tamilmani1989 tamilmani1989 approved these changes

@rjdenney rjdenney rjdenney approved these changes

@nairashu nairashu Awaiting requested review from nairashu nairashu was automatically assigned from Azure/azure-sdn-members

@rbtr rbtr Awaiting requested review from rbtr

Assignees
No one assigned
Labels
cni Related to CNI.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@paulyufan2 @vipul-21 @tamilmani1989 @ramiro-gamarra @rjdenney