-
Notifications
You must be signed in to change notification settings - Fork 344
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
Fix service port naming convention #1368
Fix service port naming convention #1368
Conversation
lujiajing1126
commented
Jan 18, 2021
•
edited
Loading
edited
- Reproduce the issue with e2e test
- Fix unit tests
- Fix issue Bad naming convention for service name #1360
Signed-off-by: Megrez Lu <[email protected]>
Signed-off-by: Megrez Lu <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #1368 +/- ##
=======================================
Coverage 86.22% 86.22%
=======================================
Files 90 90
Lines 5146 5146
=======================================
Hits 4437 4437
Misses 539 539
Partials 170 170
Continue to review full report at Codecov.
|
Signed-off-by: Megrez Lu <[email protected]>
Signed-off-by: Megrez Lu <[email protected]>
@jpkrohling I have added an e2e with istio integration which can reproduce the issue #1360 |
That looks awesome! I'm not sure the CI will be able to handle that, but it would be great if this works. |
…ction Signed-off-by: Megrez Lu <[email protected]>
Signed-off-by: Megrez Lu <[email protected]>
Now everything is fine. Besides, I am worried about this issue istio/istio#16282. I guess we also need an e2e case for gRPC over tls. |
@kevinearls would you like to review this one? |
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.
In general this looks pretty good. I've requested replacing some "if err" blocks with calls to stretchr require as that makes code more concise and gives better error information.
As the test method is quite long I've requested refactoring a couple of wait blocks into functions, one which can stay in this file but another which should go to utils.go as it is also used by other tests.
Signed-off-by: Megrez Lu <[email protected]>
Signed-off-by: Megrez Lu <[email protected]>
Signed-off-by: Megrez Lu <[email protected]>
Signed-off-by: Megrez Lu <[email protected]>
Signed-off-by: Megrez Lu <[email protected]>
Signed-off-by: Megrez Lu <[email protected]>
Signed-off-by: Megrez Lu <[email protected]>
Signed-off-by: Megrez Lu <[email protected]>
Signed-off-by: Megrez Lu <[email protected]>
Signed-off-by: Megrez Lu <[email protected]>
Signed-off-by: Megrez Lu <[email protected]>
Signed-off-by: Megrez Lu <[email protected]>
Signed-off-by: Megrez Lu <[email protected]>
Signed-off-by: Megrez Lu <[email protected]>
Signed-off-by: Megrez Lu <[email protected]>
Any other comment? |
@lujiajing1126 No not from me. Once you take out that error parameter I'll approve the PR. |
Signed-off-by: Megrez Lu <[email protected]>
Thanks for pointing that out, I even didn't notice that. Fixed. |
@jpkrohling Could you pls have a final check? Or any other comment? |
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.
If @kevinearls approved, that's good enough for me :-) Thanks for your PR!
Thanks! But it seems the merge process has to be triggered manually since github workflow files have been changed |