-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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.
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
Networking cleanup #9157
Networking cleanup #9157
Conversation
454c6a6
to
56f59cf
Compare
/retest |
pkg/apis/kops/networking.go
Outdated
@@ -37,22 +37,22 @@ type NetworkingSpec struct { | |||
GCE *GCENetworkingSpec `json:"gce,omitempty"` | |||
} | |||
|
|||
// ClassicNetworkingSpec is the specification of classic networking mode, integrated into kubernetes | |||
// ClassicNetworkingSpec is the specification of classic networking mode, integrated into kubernetes. | |||
// Support been removed since kops 1.4. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it was Kubernetes 1.4. I don't think this information is relevant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. There are quite a few issues about when features are added/removed from kops. And given that we refer users to the spec docs, I think this is relevant to many. At least it doesn't hurt.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved
pkg/apis/kops/networking.go
Outdated
// ExternalNetworkingSpec is the specification for networking that is implemented by a Daemonset | ||
// It also uses kubenet | ||
// ExternalNetworkingSpec is the specification for networking that is implemented by a Daemonset. | ||
// Uses the kubenet networking provider. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think "networking provider" is misleading as it doesn't use the kops Kubenet provider. It uses the Kubernetes kubenet networking code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amended.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think any of these are blockers. I'd like to get responses before signing off.
cmd/kops/create_cluster.go
Outdated
return true | ||
} | ||
return false | ||
return n.Kubenet == nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This previously returned false
for External
. Now it returns true
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think true is more correct. Given that both GCP and Kopeio supports private topology, it is up to the user to configure things correctly if they choose to use external.
* Remove classic from cli docs. Add missing providers * Use cilium instead of weave in example since we don't consider weave stable
Co-authored-by: Peter Rifel <[email protected]>
a4882ad
to
7259f4e
Compare
7259f4e
to
fc0f7f2
Compare
Co-authored-by: John Gardiner Myers <[email protected]>
e643959
to
1698069
Compare
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: olemarkus, rifelpet The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
No description provided.