Skip to content
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

[#5355] export Paimon jdbc user and password to properites #5376

Merged
merged 1 commit into from
Oct 31, 2024

Conversation

FANNG1
Copy link
Contributor

@FANNG1 FANNG1 commented Oct 30, 2024

What changes were proposed in this pull request?

export paimon jdbc user and password to properites

Why are the changes needed?

Fix: #5355

Does this PR introduce any user-facing change?

no

How was this patch tested?

existing IT

@FANNG1 FANNG1 marked this pull request as draft October 30, 2024 12:07
@FANNG1 FANNG1 marked this pull request as ready for review October 31, 2024 02:44
@FANNG1
Copy link
Contributor Author

FANNG1 commented Oct 31, 2024

@mchades please help to review, thanks

@mchades mchades requested a review from caican00 October 31, 2024 02:49
@@ -128,13 +128,13 @@ public class PaimonCatalogPropertiesMetadata extends BaseCatalogPropertiesMetada
"Gravitino Paimon catalog jdbc user",
false /* immutable */,
null /* defaultValue */,
true /* hidden */),
false /* hidden */),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any test to cover this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, is it nessecery to add the specific test for Paimon JDBC catalog hidden properties?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a Paimon-JDBC catalog IT? If so, it would be easy to add the properties assert tests. Otherwise, we should supplement the IT.

Copy link
Contributor Author

@FANNG1 FANNG1 Oct 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not the mater of easy or not, I think there is no need to add this to IT, since it's minor changes

Copy link
Collaborator

@caican00 caican00 Oct 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i checked the MysqlCatalog, PGCatalog, and tec. None of them tested whether the jdbc user and password were hidden or not.

I think this pr can not include the test of this modification, but a new pr can be created to specifically improve them. WDYT? cc @mchades @FANNG1

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@caican00
Copy link
Collaborator

should we mask the jdbc user and password on the gravitino ui?

@mchades
Copy link
Contributor

mchades commented Oct 31, 2024

should we mask the jdbc user and password on the gravitino ui?

I think need. @LauraXia123 cc

@LauraXia123
Copy link
Collaborator

should we mask the jdbc user and password on the gravitino ui?

I think need. @LauraXia123 cc

image the password is masked on ui

@mchades mchades added the branch-0.7 Automatically cherry-pick commit to branch-0.7 label Oct 31, 2024
@mchades mchades merged commit 1362da0 into apache:main Oct 31, 2024
26 checks passed
github-actions bot pushed a commit that referenced this pull request Oct 31, 2024
### What changes were proposed in this pull request?

export paimon jdbc user and password to properites

### Why are the changes needed?
Fix: #5355 

### Does this PR introduce _any_ user-facing change?
no

### How was this patch tested?
existing IT
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch-0.7 Automatically cherry-pick commit to branch-0.7
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug report] Open paimon catalog edit page,"jdbc-user","jdbc-password" are not filled, details page either.
4 participants