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

[Revert the revert] Change ingress fetching to be isolated per provider #116

Conversation

LiorLieberman
Copy link
Member

Reverts #114 which is the revert of #112

I copy pasted the below PR description from #112

What type of PR is this?
/kind feature

What this PR does / why we need it:
Currently we fetch all the ingresses in the main package and we pass them to all the providers.

This is not only redundant (as every provider only cares about its own ingresses) but also creates some bugs. (#109)

This also increases consistency as we already fetch CRDs at the provider level and store them in a local storage.

Note: This PR is likely to yield some more issues and TODOs that wont be addressed in this PR.
For example changing ToGatewayAPI interface function signature that wont need i2gw.InputResources anymore.
Another thing would be to revisit i2gw.InputResources struct and check if we need it or we will change it to just a list of Ingresses as it is the only things it holds now.

I structured the commits to ease the review so you could review each commit independently

Which issue(s) this PR fixes:
Fixes #109

Does this PR introduce a user-facing change?:

Move ingress fetching logic to be isolated, per provider

@k8s-ci-robot k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Jan 2, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: LiorLieberman

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

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 2, 2024
@k8s-ci-robot k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jan 2, 2024
Copy link
Member

@levikobi levikobi left a comment

Choose a reason for hiding this comment

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

/lgtm
/hold in case @mlavacca or @dpasiukevich have any further comments.

The only thing that is missed in my opinion is having a test that actually verifies the #109 bug doesn't happen anymore.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 3, 2024
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 3, 2024
@dpasiukevich
Copy link
Member

/lgtm

The only thing that is missed in my opinion is having a test that actually verifies the #109 bug doesn't happen anymore.

+1 to the test, would be great to have to avoid similar regression in future.

@LiorLieberman
Copy link
Member Author

@mlavacca is off for a bit, and we agreed on slack that testing this would be slightly tricky because of the provider initialization.

removing the hold

/unhold

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 4, 2024
@k8s-ci-robot k8s-ci-robot merged commit 5a49d40 into kubernetes-sigs:main Jan 4, 2024
2 checks passed
xtineskim pushed a commit to xtineskim/ingress2gateway that referenced this pull request Jan 4, 2024
…er (kubernetes-sigs#116)

* pass namespace in ProviderConf and make read from file account for namepsaces

* move ingress-nginx to fetch its own ingresses

* move Kong to fetch its own ingresses

* Deprecate ingress fetching from the generic package

Also added issue numbers for TODOs

* add helper common functions for reading ingresses and extract them from file
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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", 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.

Duplicate resources when using implementation-specific annotations
4 participants