fix credentials getting printed twice#1682
Merged
flyrain merged 1 commit intoapache:mainfrom May 28, 2025
Merged
Conversation
XN137
commented
May 27, 2025
| for (String realm : realms) { | ||
| RealmContext realmContext = () -> realm; | ||
| if (!metaStoreManagerMap.containsKey(realmContext.getRealmIdentifier())) { | ||
| if (!metaStoreManagerMap.containsKey(realm)) { |
Contributor
Author
There was a problem hiding this comment.
decided to refactor this for clarity as the realmContext is built from realm right above
Contributor
Author
|
wondering about this code: whether it should also only print credentials that were not taken from the root credentials as is done by QuarkusProducers.maybeBootstrap |
XN137
requested review from
HonahX,
MonkeyCanCode,
RussellSpitzer,
adutra,
ajantha-bhat,
ashvina,
collado-mike,
dennishuo,
dimas-b,
ebyhr,
eric-maynard,
flyrain,
jackye1995,
jbonofre,
pingtimeout,
singhpk234,
snazy,
takidau and
vvcephei
as code owners
dimas-b
approved these changes
May 27, 2025
Contributor
dimas-b
left a comment
dimas-b
left a comment
There was a problem hiding this comment.
Thanks for looking into this, @XN137 !
Not printing credentials to STDOUT in metastore implementations makes sense to me (even for the in-memory case).
I also support the idea of handling all initial realm setup in runtime-specific code (QuarkusProducers in this case) and the Admin tool.
github-project-automation
bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
adnanhemani
approved these changes
May 27, 2025
Contributor
adnanhemani
left a comment
adnanhemani
left a comment
There was a problem hiding this comment.
Agreed with @dimas-b ! LGTM!
flyrain
approved these changes
May 28, 2025
github-project-automation
bot
moved this from Ready to merge
to Done
in Basic Kanban Board
Contributor
|
Thanks @XN137 for the fix. Thanks everyone for the review. |
snazy
added a commit
to snazy/polaris
that referenced
this pull request
Jun 13, 2025
* fix: Remove duplicated code in IcebergCatalog (apache#1681) * Fix SparkClient listGenericTable to use ListGenericTablesRESTResponse ListTableResponse is the class used by iceberg endpoint, which is the same as ListGenericTablesRESTResponse. However, we are suppose to use ListGenericTablesRESTResponse to be correct * fix: Remove info log about deprecated internal method from PolarisConfiguration (apache#1672) Remove the INFO log about calls to this method in Polaris, because users cannot do anything about these messages. Phasing out old property names requires coordination with users (e.g. release notes), so it is not a matter of merely avoiding calls to that method in Polaris code. Fixes apache#1666 * Create a single binary distribution bundle (apache#1589) * fix(quickstart): Correct Quickstart Instructions (apache#1673) * Remove Java URI validations for Blob Storage providers (apache#1604) This is a retry at apache#1586, which I'll close if this PR is on the right direction instead. Java URI does not actually apply any normalization to URIs if we do not call URI.normalize() (which we currently do not). Additionally, blob storage providers like S3 and GCS can provide ".." and "." as valid fragments in URLs - which Java URI would attempt to normalize incorrectly. As a result, attempting to validate and/or normalize URIs for blob storage providers using the Java URI class is the incorrect behavior. While we may want to add location validation via regex later, removing it first should at least unblock the bug we see in apache#1545. * Improve test coverage for invalid inputs in Policy APIs (apache#1665) * Fix getting-started docker start by PR apache#1532 (apache#1687) * Fix the manual test broken by PR apache#1532 (apache#1688) * Fix credentials printing twice (apache#1682) * main: Update dependency com.diffplug.spotless:spotless-plugin-gradle to v7.0.4 (apache#1690) * INFO: last merged commit 493de03 --------- Co-authored-by: Alexandre Dutra <[email protected]> Co-authored-by: gh-yzou <[email protected]> Co-authored-by: Dmitri Bourlatchkov <[email protected]> Co-authored-by: Yufei Gu <[email protected]> Co-authored-by: Adnan Hemani <[email protected]> Co-authored-by: William Hyun <[email protected]> Co-authored-by: Christopher Lambert <[email protected]> Co-authored-by: Mend Renovate <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
after #1376 a comment shows that non-root credentials could get printed twice:
so we remove all logging/printing from the
bootstrapRealmsimplementations as the outer callersQuarkusProducers.maybeBootstrapandBootstrapCommand.callare already taking care of that.