Use excludeHeaderList instead of blackListHeaders #470
Conversation
Chaho12
force-pushed
the
feature/fix-spelling
branch
from
92de567
to
b85acc6
Compare
Chaho12
changed the title
gateway-ha/src/main/java/io/trino/gateway/ha/config/RulesExternalConfiguration.java
Outdated
Show resolved
Hide resolved
|
Agreed .. this is a good change @Chaho12 |
Chaho12
force-pushed
the
feature/fix-spelling
branch
from
b85acc6
to
a268dc5
Compare
gateway-ha/src/main/java/io/trino/gateway/ha/router/ExternalRoutingGroupSelector.java
Outdated
Show resolved
Hide resolved
gateway-ha/src/main/java/io/trino/gateway/ha/router/ExternalRoutingGroupSelector.java
Outdated
Show resolved
Hide resolved
| @@ -84,7 +84,7 @@ static Stream<RulesExternalConfiguration> provideRoutingRuleExternalConfig() | |||
| { | |||
| RulesExternalConfiguration restConfig = new RulesExternalConfiguration(); | |||
| restConfig.setUrlPath("http://localhost:8080/api/public/gateway_rules"); | |||
| restConfig.setBlackListHeaders(new ArrayList<>(List.of("Authorization"))); | |||
| restConfig.setExcludeHeaders(new ArrayList<>(List.of("Authorization"))); | |||
There was a problem hiding this comment.
Why are we calling new ArrayList here? Also, why does this test class use @ParameterizedTest just for single case?
There was a problem hiding this comment.
Why are we calling new ArrayList here?
True a list is sufficent.
Also, why does this test class use @ParameterizedTest just for single case?
what do you mean by a single case?
There was a problem hiding this comment.
I meant this method returns a single RulesExternalConfiguration. We shouldn't use @ParameterizedTest in such a case.
There was a problem hiding this comment.
I am not that familiar with Junit 5 so please bear with me.
Well, I wanted to reduce code duplication for configs, as it could be used several times.
And i thought this way of using parameterized test makes it concise to test.
But now that you mention, since this is not complex and returns as a single object,
i could simply create this object in initialize() function, make it where it is @BeforeAll. right?
Thanks for writing a release note entry. I would recommend mentioning where the |
|
please rebase and address the few small comments @Chaho12 .. then we can merge |
Chaho12
force-pushed
the
feature/fix-spelling
branch
from
a268dc5
to
f9a704d
Compare
Description
Use excludeHeaders instead as blacklist isn't recommended in software projects.
Additional context and related issues
Release notes
( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
(x) Release notes are required, with the following suggested text: