Rename AccessConfig and AccessConfigProvider for clarity #2883
Conversation
There was a problem hiding this comment.
@CodingBangboo Thanks for the contribution! The rename LGTM as it would help avoid confusion when we consolidate and introduce CatalogAccessConfig in the future, per comment.
Would love to hear others' thoughts on this!
github-project-automation
bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
dimas-b
left a comment
dimas-b
left a comment
There was a problem hiding this comment.
This rename looks reasonable to me 👍 Thanks for your contribution, @CodingBangboo !
| * Constructor for success | ||
| * | ||
| * @param accessConfig credentials | ||
| * @param storageAccessConfig credentials |
There was a problem hiding this comment.
nit: this javadoc is no longer accurate. Since the class name is pretty much self-describing now, I do not see a reason to have javadoc for this parameter (can we put anything there that is not covered by the class?).
There was a problem hiding this comment.
Good point. I think we can keep it there for consistency with the javadoc in other BaseResult classes. I also added a tiny bit to indicate that it is the result from a successful getSubscopedCredsForEntity().
dimas-b
changed the title
CodingBangboo
force-pushed
the
nuoya_rename_AccessConfig
branch
from
144700b to
558d524
Compare
| * Constructor for success | ||
| * | ||
| * @param accessConfig credentials | ||
| * @param storageAccessConfig credentials generated by a successful getSubscopedCredsForEntity() |
There was a problem hiding this comment.
As far as this class is concerned, why does it matter that storageAccessConfig is generated by a successful getSubscopedCredsForEntity()?
There was a problem hiding this comment.
@CodingBangboo @HonahX : I know this is a minor point, but I'd prefer to avoid false requirements in javadoc... WDYT about this one?
To be clear: I propose to remove this javadoc line completely. I believe the java doc on the parameter type is sufficient in this case.
4 participants
What changes were proposed in this pull request?
As a follow up of #2736 (comment), we would like to rename the
AccessConfigandAccessConfigProvidertoStorageAccessConfigandStorageAccessConfigProviderrespectively, including renamingAccessConfigProvider's methodgetAccessConfigtogetStorageAccessConfig. This renaming PR adds clarity to differentiating between two types of external services: storage and catalog accesses.Why are the changes needed?
Does this PR introduce any user-facing change?
How was this patch tested?
CHANGELOG.md