-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Add annotationFilter to Ambassador Hosts #2043
Add annotationFilter to Ambassador Hosts #2043
Conversation
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
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. I understand the commands that are listed here. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: RicardoNalesAmato The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
3b52fce
to
3b26557
Compare
3b26557
to
f4606a0
Compare
/kind feature |
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.
Can we add tests to this PR to test the filtering functionality?
Sure, I'll add that as soon as possible. |
12ac3e6
to
846f7b1
Compare
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.
Hello hello 👋🏻 , in general I'm fine merging this, however I'd like to see some tests added to the changes you made. Do you mind adding some tests to see if that works like expected?
Thank you ❤️
846f7b1
to
46805e3
Compare
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-contributor-experience at kubernetes/community. |
46805e3
to
f33c180
Compare
/remove-lifecycle stale |
f33c180
to
5761f8a
Compare
@njuettner @Raffo I completely forgot about this PR! I will be adding the tests this week. Are you guys still ok with merging this? |
Sorry I was just searching the PRs for Ambassador and came across this one. I also created a PR for this feature as I only searched the issues 🤦♂️ to see if anyone else had issues with the annotation filter not working. I have added a bunch of tests to my PR so you can steal them if you would like. #2633 |
Thanks @KyleMartin901 . If the implementation is the same and you've already added the tests I'd say let's try to get your PR merged instead. |
5761f8a
to
ed5a26d
Compare
This commit allows users to filter the Host resources based on a given annotation using `--annotation-filter`. This commit uses the same `AnnotationFilter` that is already available (so a new type was not added) and already used by many other source types. Signed-off-by: Ricardo Nales Amato <[email protected]>
ed5a26d
to
e57d3cd
Compare
@RicardoNalesAmato, our implementation is nearly identical. The I am happy either way if we use yours or mine would just love to have it fixed so won't have to maintain a fork with the patch 😀 You were first and have interacted with the team so happy to just have the tests added to your implementation so you maintain the credit for your work. |
Thanks, @KyleMartin901! I'll copy your tests later today and will ping the team afterwards. |
@RicardoNalesAmato : any particular reason why annotations are used for the filtering on the resources? Except for the historical one :) |
@alebedev87 currently the label filter doesn't support the Ambassador Host resource type are you suggesting modifying the PR to allow the Host resource type via the label filter? For me, I missed that the label filter had been added as an option I was modelling my implementation off of the 3rd party sources like the Isto Gateway, and Contour HTTP Proxy that didn't have it enabled and I had only been using the annotation filter so totally missed it 😞. I copied the existing implementations of 3rd party sources like the Isto Gateway, Contour HTTP Proxy etc. Should all the 3rd party sources be added to the label filter each individually by essentially copying what was done for service and ingress as done in the PR d91b7e6 |
@alebedev87 I just pushed an update to my PR f10f6eb so it now includes the label filter is that what you were looking for. |
@KyleMartin901 : yes However when I looked at the unit tests and saw that |
Hey @KyleMartin901 I was more busy than I expected last week and forgot to update this PR. Would you like to go with your PR instead as I see you’ve made some more changes, or shall I copy them to this one? @alebedev87 In our case having that annotation greatly reduced the complexity of our Nginx to Ambassador migration. I believe that is the main reason why all other implementations Did the same thing. Could we perhaps move forward with this solution and refine it at a later date? |
@RicardoNalesAmato i don't mind as long as we can get either one added I'll be super happy. If you don't have time I'm happy to go with mine but also happy if you want to cherry-pick the label implementation and just add the annotation tests. |
@alebedev87 if you have any more information on what you like done to be able to get my PR merged in so an AmbassadorHost is able to be filtered would be awesome. I used the ingress.class annotation within the tests just so the tests matched the other sources and the examples used for the AWS provider as I presumed most people would be looking at those for how it should be used, so I used them to keep it constant within the External DNS project. The Ambassador Host resource itself doesn't use the ingress.class annotation to create an ingress route you must define an Ambassador Host and an Ambassador Mapping resource for any ingress to be created. |
@KyleMartin901: cool, thanks for addressing my remark! I left a small review. |
This commit allows users to filter the Host resources based on
a given annotation using
--annotation-filter
.This commit uses the same
AnnotationFilter
that isalready available (so a new type was not added) and already used
by many other source types.