Add gcs.auth-type configuration
#26681
Conversation
|
I've restored compatibility mode for |
| // FALSE is mapped to DEFAULT | ||
| public static AuthType fromString(String value) | ||
| { | ||
| return switch (value.toUpperCase(ENGLISH)) { |
There was a problem hiding this comment.
hacky. I also wanted backward compatibility here but I think deprecating setUseAccessToken but still keeping it. And add validation that setUseAccessToken is not used together with setAuthType would be cleaner
There was a problem hiding this comment.
- +1 for the deprecation ( @ Deprecated and even @ ConfigHidden) and mutual exclusiveness of those properties
- should we add the ticket with next release after this one introducing new property for removing the deprecated property
There was a problem hiding this comment.
@marcinsbd Agree with on " add the ticket with next release after this one introducing new property", I am going to add a draft pr for it, and wait current release finalized
chenjian2664
changed the title
gcs.use-access-token with gcs.auth-type configurationgcs.auth-type configuration
| @Config("gcs.use-access-token") | ||
| public GcsFileSystemConfig setUseGcsAccessToken(boolean useGcsAccessToken) | ||
| { | ||
| checkArgument(authType == null, "Cannot set both gcs.use-access-token and gcs.auth-type"); |
There was a problem hiding this comment.
What if the authType is set after user-access-token? This will override the value without clear indication that this shouldn't be called. I think that this approach is not superior to the one I've already proposed.
There was a problem hiding this comment.
This is not how validation works.
It is run after all setters are run and we could model it in such way that you know that both setters were called - and that should result in failure.
There was a problem hiding this comment.
So the setUseGcsAccessToken should be optional in that case
There was a problem hiding this comment.
Yeah - implementation wise changing useGcsAccessToken from boolean to Optional<Boolean> seems natural if we want to go this path.
There was a problem hiding this comment.
In TestGcsFileSystemConfig#testDefaults we need use set method explicitly, do you have idea how to check those two property?
There was a problem hiding this comment.
testing defaults should be fine. you just assert on getters.
As for testing explicit setting is a bit more tricky. But recently we added assertFullMapping(Map<String, String> properties, T expected, Set<String> skipped) - so you can test scenarios when one setter is used, and then when the other is used.
This makes it easier to add new GCS authentication types in the future. The commit also deprecate `gcs.use-access-token`
chenjian2664
force-pushed
the
gcs-auth-type
branch
from
19ece4c to
4e57b84
Compare
This makes it easier to add new GCS authentication types in the future
also deprecated
gcs.use-access-tokenDescription
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: