-
Notifications
You must be signed in to change notification settings - Fork 360
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
refactor: remove explicit test case definition for xds translator test #3230
Conversation
Signed-off-by: shawnh2 <shawnhxh@outlook.com>
Signed-off-by: shawnh2 <shawnhxh@outlook.com>
{ | ||
name: "ratelimit", | ||
"multiple-listeners-same-port": { | ||
requireSecrets: true, |
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 this check also be removed, and secrets be asserted if a secrets file exists ?
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.
good idea!
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.
by removing requireSecrets
and checking secrets from translator result, I found some xxx.secret.yaml
were missing, so added in this PR as well
@@ -14,26 +15,6 @@ | |||
initialStreamWindowSize: 65536 | |||
maxConcurrentStreams: 100 | |||
httpFilters: | |||
- name: envoy.filters.http.jwt_authn |
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.
why is this changing ?
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.
because this test file has been ignored (not explicit defined in test cases) until now, its content is outdated.
Signed-off-by: shawnh2 <shawnhxh@outlook.com>
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.
LGTM thanks !
What type of PR is this?
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #2229