-
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 new source for istio virtual services #1607
Conversation
Not sure if the failures are related to this PR. @sheerun I believe the ticker is being triggered twice and because of that, we are sending to a closed channel which causes the panic in source_test.go |
@tariq1890 This is a dup of #1358. I didn't open another PR myself because @devkid did most of the work already.. |
@tariq1890 Could you try to fix this test? |
@elsesiy If you see that PR thread, I had followed up and I hadn't heard back in 10 days. He will be added as a co-author of this PR of course |
@tariq1890 Sure I understand. Why don’t you preserve the commits from the original PR? |
I usually squash my commit history and like to keep it clean that way. Plus it was easier making the changes manually instead of git rebasing as a lot of changes have been made since that PR was created |
9b2e3dc
to
0176a7a
Compare
That's fine but in my opinion you should add everybody who contributed code to this as co-author then :) |
@elsesiy Done! |
@thomasv314 Requesting you to test this PR on your clusters. It would be good to confirm if |
@tariq1890 if i have bandwidth today i will give it a go on our development cluster |
@njuettner @Raffo Requesting your review on this. |
@thomasv314 Were you able to test this? |
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 added a few comments, I'm definitely not a istio expert, but the changes do look good.
Is there anything that we can do to validate this, for example by testing it in prod @thomasv314 ?
}, | ||
) | ||
|
||
// TODO informer is not explicitly stopped since controller is not passing in its channel. |
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.
What can we do to fix that?
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 is another copy pasta from the existing gateway source. Unfortunately, I haven't looked into this too much. I can tackle this is another PR if that's okay with you
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.
Gotcha, works.
@tariq1890 are you able to address the comments? |
@Raffo Thanks for the follow-up. I have addressed your comments. |
ping ping @njuettner @Raffo I have tested this in my development cluster and can confirm that it works |
@steerben Have you tested this branch? |
If we can get an additional +1 from @steerben or anyone else I'm gonna approve for merge. |
I will give it a run again this evening and will respond! @tariq1890 @Raffo |
Hey! I got some issues: Build went through but tests failed in the docker build: Furthermore I only used Fix has to be made in I fixed that on my end and it set the DNS entries corrrectly. |
Co-authored-by: Alfred Krohmer <alfred.krohmer@logmein.com> Co-authored-by: Jonas-Taha El Sesiy <github@elsesiy.com>
@steerben Thank you so much for trying this out! I have fixed the flag issues, but I was not able to replicate those test failures. They seem to be running fine in and out of the docker context. |
@steerben @thomasv314 Would one of you be able to sign off on this? |
Fine for me. :) |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Raffo, tariq1890 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 |
Raising this PR since we would really love to test this feature out in our Istio setup. I have seen that there is a lot of request for this PR to be merged.
This is a dup of #1358. Thanks to @devkid and @elsesiy for their efforts in creating this
Closes #1340
Closes #1358