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.

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

helper funcs for route reconciler to construct ClusterIngress #2298

Merged
merged 2 commits into from
Oct 25, 2018

Conversation

lichuqiang
Copy link
Contributor

Part of #1963
Follow-up #2189

Proposed Changes

  • helper funcs to construct clusteringress
  • create placeholder service according to ingress status

Release Note

update route reconciler to create ClusterIngress instead of VirtualService

/area networking

/assign @tcnghia @mattmoor

@knative-prow-robot knative-prow-robot added area/networking size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 24, 2018
Copy link
Contributor

@knative-prow-robot knative-prow-robot left a comment

Choose a reason for hiding this comment

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

@lichuqiang: 0 warnings.

In response to this:

Part of #1963
Follow-up #2189

Proposed Changes

  • helper funcs to construct clusteringress
  • create placeholder service according to ingress status

Release Note

update route reconciler to create ClusterIngress instead of VirtualService

/area networking

/assign @tcnghia @mattmoor

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

// The purpose of this service is to provide a domain name for Istio routing.
// TODO: It is to replace the "MakeK8sService" func after route reconciler is switched to reconcile
// ClusterIngress.
func NewMakeK8sService(route *v1alpha1.Route, ingress *netv1alpha1.ClusterIngress) (*corev1.Service, error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here I rename the new func to avoid conflict.
Will remove the old one once we finally switch to the new pattern in reconciler.

@knative-metrics-robot
Copy link

The following is the coverage report on pkg/.
Say /test pull-knative-serving-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/reconciler/v1alpha1/route/resources/cluster_ingress.go Do not exist 100.0%

Copy link
Contributor

@tcnghia tcnghia left a comment

Choose a reason for hiding this comment

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

/approve

@@ -159,48 +159,6 @@ func TestMakeVirtualServiceSpec_CorrectRoutes(t *testing.T) {
}
}

func TestGetRouteDomains_NamelessTarget(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just move these tests to cluster_ingress_test.go instead of deleting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we have (see cluster_ingress_test.go#L137).

@knative-prow-robot knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 24, 2018
@tcnghia
Copy link
Contributor

tcnghia commented Oct 25, 2018

/lgtm
/approve

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 25, 2018
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lichuqiang, tcnghia

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@mattmoor
Copy link
Member

Thanks for splitting this off @lichuqiang !

@knative-prow-robot knative-prow-robot merged commit 0873d0a into knative:master Oct 25, 2018
@lichuqiang lichuqiang deleted the route_help_func branch October 26, 2018 01:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/networking lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants