Skip to content

Conversation

@zhangming870
Copy link
Contributor

Reason for Change:

replace NumCPU with NumCores, so that it can be correctly decode in dnc side.

Issue Fixed:

wrong request field name, should use NumCores instead of NumCPU

Requirements:

Notes:

@zhangming870 zhangming870 requested a review from a team as a code owner May 3, 2022 20:55
@zhangming870 zhangming870 requested review from rbtr and removed request for a team May 3, 2022 20:55
@rbtr
Copy link
Collaborator

rbtr commented May 3, 2022

@zhangming870 what's the context? has this just always been wrong and silently ignored, or has something changed in DNC?

@zhangming870
Copy link
Contributor Author

@zhangming870 what's the context? has this just always been wrong and silently ignored, or has something changed in DNC?

@rbtr We believe it always been wrong, because no one uses it yet. This is for dnc in managed mode.

@zhangming870 zhangming870 added cns Related to CNS. and removed do-not-merge labels May 3, 2022
@zhangming870
Copy link
Contributor Author

good to merge

@rbtr rbtr added the fix Fixes something. label May 4, 2022
@rbtr
Copy link
Collaborator

rbtr commented May 4, 2022

@zhangming870 can you rebase? there was a fix to the CI which should improve your check results 🙂

@zhangming870
Copy link
Contributor Author

@rbtr sure. rebased.

@rbtr rbtr enabled auto-merge (squash) May 5, 2022 04:59
@rbtr rbtr disabled auto-merge May 5, 2022 04:59
@rbtr
Copy link
Collaborator

rbtr commented May 5, 2022

@zhangming870 looks like something messed up the commit history: https://github.com/Azure/azure-container-networking/pull/1358/commits
can you try? happy to help if you need it. looks like you might have rebased locally, but then merged the remote branch back in to your local branch. after a rebase, you need to force push because you have rewritten the branch history.

Copy link
Collaborator

@rbtr rbtr left a comment

Choose a reason for hiding this comment

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

commit history needs to be fixed

@zhangming870
Copy link
Contributor Author

@rbtr rebased again and did a sync branch, I guess it pushed history to remote. thanks for letting me know how to do that. And please let me know if anything still wrong.

@zhangming870 zhangming870 force-pushed the ming/registerNodeBugfix branch from 88c7ba5 to 183fd0a Compare May 17, 2022 16:49
rbtr
rbtr previously approved these changes May 17, 2022
@zhangming870 zhangming870 enabled auto-merge (squash) May 17, 2022 16:53
@rbtr
Copy link
Collaborator

rbtr commented May 26, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@zhangming870 zhangming870 merged commit 85c0fe0 into master Jun 16, 2022
@zhangming870 zhangming870 deleted the ming/registerNodeBugfix branch June 16, 2022 16:24
matmerr pushed a commit to matmerr/azure-container-networking that referenced this pull request Jun 29, 2022
… in dnc side (Azure#1358)

Co-authored-by: Ming Zhang <zhangming@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cns Related to CNS. fix Fixes something.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants