Skip to content
This repository has been archived by the owner on Jan 11, 2023. It is now read-only.

Set Windows Azure CNI config #2822

Merged
merged 2 commits into from
May 4, 2018
Merged

Conversation

saiyan86
Copy link
Contributor

@saiyan86 saiyan86 commented May 1, 2018

What this PR does / why we need it:
This PR enables kubernetes DNS and endpoint policies in Azure CNI.

Special notes for your reviewer:
This PR requires Azure CNI v1.0.4. Please do not merge until Azure CNI v1.0.4 is released.
Required Azure CNI PR: Azure/azure-container-networking#127

@saiyan86
Copy link
Contributor Author

saiyan86 commented May 1, 2018

@JiangtianLi @tamilmani1989 @sharmasushant Could you please review? Thanks!

@JiangtianLi
Copy link
Contributor

@saiyan86 It is hard to see the diff, possibly due to line ending? Can you adjust it?

@jackfrancis
Copy link
Member

@JiangtianLi append ?w=1 to the end of the "Files changed" URL to remove whitespace changes.

@saiyan86 I've tagged this PR as do-not-merge. Please @ me when v1.0.4 is released and I'll move this effort forward. Thanks!

JiangtianLi
JiangtianLi previously approved these changes May 1, 2018
Copy link
Contributor

@JiangtianLi JiangtianLi left a comment

Choose a reason for hiding this comment

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

/lgtm

@codecov
Copy link

codecov bot commented May 1, 2018

Codecov Report

Merging #2822 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2822   +/-   ##
=======================================
  Coverage   46.94%   46.94%           
=======================================
  Files          86       86           
  Lines       12809    12809           
=======================================
  Hits         6013     6013           
  Misses       6241     6241           
  Partials      555      555

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8123237...1bfe94f. Read the comment docs.

@codecov
Copy link

codecov bot commented May 1, 2018

Codecov Report

Merging #2822 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #2822   +/-   ##
======================================
  Coverage    49.3%   49.3%           
======================================
  Files          91      91           
  Lines       13803   13803           
======================================
  Hits         6805    6805           
  Misses       6368    6368           
  Partials      630     630
Impacted Files Coverage Δ
pkg/acsengine/defaults.go 73.78% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 057b045...0c04344. Read the comment docs.

@saiyan86
Copy link
Contributor Author

saiyan86 commented May 1, 2018

@JiangtianLi Sorry for the hassle, but the line ending seems to be fine with me.
I don't know why git treats all of them as difference.

@JiangtianLi
Copy link
Contributor

@saiyan86 no problem. what link do you use to see the diff? Just curious what have changed recently. I didn't remember to append ?w=1 before.

@saiyan86
Copy link
Contributor Author

saiyan86 commented May 2, 2018

@JiangtianLi I added a Set-AzureCNIConfig() function and calls it in Set-NetworkConfig
.

@JiangtianLi
Copy link
Contributor

@saiyan86 Sorry I didn't mean what you have changed. I wonder if you can view the diff without ?w=1?

@saiyan86
Copy link
Contributor Author

saiyan86 commented May 2, 2018

@JiangtianLi I can view the diff in my local VSCode. But for github, I have to add ?w=1.

@saiyan86
Copy link
Contributor Author

saiyan86 commented May 4, 2018

@JiangtianLi @jackfrancis We just released Azure CNI v1.0.4. Could you please pull the binaries to acs-engine mirror and merge this PR? Thanks!!

@ghost ghost assigned jackfrancis May 4, 2018
@ghost ghost added the in progress label May 4, 2018
@jackfrancis
Copy link
Member

@saiyan86 I updated defaults.go to use the new version of Azure CNI. Let's run it through E2E.

@jackfrancis
Copy link
Member

@saiyan86 Is there any additional Windows work that needs to happen for this PR, or should we assume successful E2E deployments indicate validation of the changes?

@PatrickLang
Copy link
Contributor

If you can point me to a build (circleCI?) I can do a quick deploy

Copy link
Member

@jackfrancis jackfrancis left a comment

Choose a reason for hiding this comment

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

lgtm

@jackfrancis jackfrancis merged commit 1b7da0a into Azure:master May 4, 2018
@ghost ghost removed the in progress label May 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants