Change file structure for ingresses by houshengbo · Pull Request #1355 · knative/operator

houshengbo · 2023-02-27T20:00:26Z

Fixes #1345

Proposed Changes

This PR aimed to change the selection mode for the ingresses from a filter based mode to a directory based mode. We can pick up the ingress by the name of the ingress from the designated directory

Release Note

knative-prow · 2023-02-27T20:00:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: houshengbo

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [houshengbo]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

knative-prow

@houshengbo: 0 warnings.

Details

In response to this:

Fixes #1345

Proposed Changes

WIP

Release Note

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.

linux-foundation-easycla · 2023-04-11T04:14:24Z

The committers listed above are authorized under a signed CLA.

✅ login: houshengbo / name: Vincent (d9e34b7, 5600cde)

…ructure

codecov · 2023-04-11T14:32:44Z

Codecov Report

Patch coverage: 81.81% and project coverage change: -0.50 ⚠️

Comparison is base (1ba80e6) 79.84% compared to head (01acb99) 79.34%.

❗ Current head 01acb99 differs from pull request most recent head bd4de72. Consider uploading reports for the commit bd4de72 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1355      +/-   ##
==========================================
- Coverage   79.84%   79.34%   -0.50%     
==========================================
  Files          38       39       +1     
  Lines        1761     1748      -13     
==========================================
- Hits         1406     1387      -19     
- Misses        257      263       +6     
  Partials       98       98

Impacted Files	Coverage Δ
pkg/reconciler/knativeserving/common/conversion.go	`0.00% <0.00%> (ø)`
pkg/reconciler/knativeserving/ingress/contour.go	`100.00% <ø> (ø)`
pkg/reconciler/knativeserving/ingress/istio.go	`71.11% <ø> (ø)`
pkg/reconciler/knativeserving/ingress/ingress.go	`92.85% <94.11%> (-1.12%)`	⬇️
pkg/reconciler/knativeserving/ingress/kourier.go	`75.34% <100.00%> (ø)`
pkg/reconciler/knativeserving/security/security.go	`77.08% <100.00%> (-2.55%)`	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

nak3 · 2023-04-12T07:32:41Z

pkg/reconciler/knativeserving/ingress/kourier.go

 	}
 }
+
+func hasProviderLabel(u *unstructured.Unstructured) bool {


We can drop hasProviderLabel in Kourier as well so please feel free to drop it. Or I will drop them in the separate PR.

OK. I can do it.

nak3 · 2023-04-12T07:38:50Z

pkg/reconciler/knativeserving/ingress/ingress.go

+	if len(urls) == 0 {
+		url := filepath.Join(ingressPath, "istio")


We have the default behavior around L#67 so it should be fine, I think. But currently (without this PR) nonFilter was returned and no ingress was created. So, it should be same behavior instead of installing Istio?

Right. Let me change it. I can add some more test cases to see if the edge cases are covered.

nak3 · 2023-04-12T07:43:35Z

pkg/reconciler/knativeserving/ingress/ingress.go

-func hasProviderLabel(u *unstructured.Unstructured) bool {
-	if _, hasLabel := u.GetLabels()[providerLabel]; hasLabel {
-		return true
+func convertToKS(instance base.KComponent) *v1beta1.KnativeServing {


Exact same function exists in ./pkg/reconciler/knativeserving/security/security.go can we change it a common func and use it?

good point.

houshengbo · 2023-04-12T15:24:34Z

/test eventing-upgrade-tests

nak3 · 2023-04-13T01:00:35Z

/cc @skonto @ReToCode

LGTM

ReToCode

Other than that, LGTM.

ReToCode · 2023-04-13T05:11:49Z

cmd/fetcher/kodata/config.yaml

  - s3:
      bucket: "gs-noauth://knative-releases"
      prefix: "net-istio/previous"
+    eventingService: istio


Is there a reason why this is called eventingService? I was first confused with Eventing as we are in the knative-serving part of the yaml.

IngressService or just ingress?

@ReToCode @skonto I changed the name of the field.

nak3 · 2023-04-17T00:30:47Z

/lgtm

knative-prow bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Feb 27, 2023

houshengbo changed the title ~~Change ingress structure~~ WIP: Change ingress structure Feb 27, 2023

knative-prow bot requested review from aliok and maximilien February 27, 2023 20:00

knative-prow bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 27, 2023

knative-prow bot reviewed Feb 27, 2023

View reviewed changes

houshengbo force-pushed the change-ingress-structure branch 3 times, most recently from fc4b47a to b923cb7 Compare February 27, 2023 22:19

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 28, 2023

houshengbo force-pushed the change-ingress-structure branch from b923cb7 to a9bb897 Compare April 11, 2023 04:14

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 11, 2023

houshengbo force-pushed the change-ingress-structure branch 2 times, most recently from a9625ca to 5600cde Compare April 11, 2023 05:04

Vincent Hou added 2 commits April 11, 2023 10:18

Change the configuration for the ingress and generate the new file st…

0b7296d

…ructure

Refactor the ingress selection based on the new file structure

b8b3331

houshengbo force-pushed the change-ingress-structure branch from 5600cde to b8b3331 Compare April 11, 2023 14:18

houshengbo force-pushed the change-ingress-structure branch 8 times, most recently from de87cd7 to 171037e Compare April 11, 2023 19:26

Fixed the issues with test cases

3c11c53

houshengbo force-pushed the change-ingress-structure branch from 171037e to 3c11c53 Compare April 11, 2023 19:32

houshengbo changed the title ~~WIP: Change ingress structure~~ Change file structure for ingresses Apr 11, 2023

knative-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 11, 2023

nak3 reviewed Apr 12, 2023

View reviewed changes

Vincent Hou added 3 commits April 12, 2023 10:36

Drop the ProviderLabel for Kourier

746807c

Remove the unused logic in getIngressPath

2d866bf

Fix the common function

01acb99

knative-prow bot requested review from ReToCode and skonto April 13, 2023 01:00

ReToCode reviewed Apr 13, 2023

View reviewed changes

Rename the field to ingressService

bd4de72

knative-prow bot assigned nak3 Apr 17, 2023

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Apr 17, 2023

knative-prow bot merged commit 2668082 into knative:main Apr 17, 2023

ReToCode added a commit to ReToCode/serverless-operator that referenced this pull request Aug 4, 2023

Use subdir for ingress manifests based on knative/operator#1355

b879613

ReToCode mentioned this pull request Aug 4, 2023

Bump all Knative go-deps to version 1.10 openshift-knative/serverless-operator#2207

Merged

ReToCode added a commit to ReToCode/serverless-operator that referenced this pull request Aug 4, 2023

Use subdir for ingress manifests based on knative/operator#1355

006568e

ReToCode added a commit to ReToCode/serverless-operator that referenced this pull request Aug 7, 2023

Use subdir for ingress manifests based on knative/operator#1355

443c6d4

ReToCode added a commit to ReToCode/serverless-operator that referenced this pull request Aug 8, 2023

Use subdir for ingress manifests based on knative/operator#1355

493cb4d

		if len(urls) == 0 {
		url := filepath.Join(ingressPath, "istio")

Conversation

houshengbo commented Feb 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Uh oh!

knative-prow bot commented Feb 27, 2023

Uh oh!

knative-prow bot left a comment

Choose a reason for hiding this comment

Proposed Changes

Uh oh!

linux-foundation-easycla bot commented Apr 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Apr 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

houshengbo commented Apr 12, 2023

Uh oh!

nak3 commented Apr 13, 2023

Uh oh!

ReToCode left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nak3 commented Apr 17, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

houshengbo commented Feb 27, 2023 •

edited

Loading

linux-foundation-easycla bot commented Apr 11, 2023 •

edited

Loading

codecov bot commented Apr 11, 2023 •

edited

Loading