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

[#5987] improvement: Add more configurations for the config API #5999

Merged
merged 5 commits into from
Dec 26, 2024

Conversation

jerqi
Copy link
Contributor

@jerqi jerqi commented Dec 26, 2024

What changes were proposed in this pull request?

Add more configurations for the config API

Why are the changes needed?

Fix #5987
Front end can use this config option to optimize the user experience.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

image

@jerqi jerqi requested a review from mchades December 26, 2024 03:25
@jerqi
Copy link
Contributor Author

jerqi commented Dec 26, 2024

cc @LauraXia123

mchades
mchades previously approved these changes Dec 26, 2024
@tengqm
Copy link
Contributor

tengqm commented Dec 26, 2024

Should the value be "[Simple]" or ["Simple"]?

@jerqi
Copy link
Contributor Author

jerqi commented Dec 26, 2024

Should the value be "[Simple]" or ["Simple"]?

You are right. It should be ["simple"].

@mchades mchades dismissed their stale review December 26, 2024 06:22

the value should be in json type not json string

@jerqi jerqi requested a review from mchades December 26, 2024 06:47
@jerqi
Copy link
Contributor Author

jerqi commented Dec 26, 2024

cc @lw-yang

@jerqi jerqi requested a review from mchades December 26, 2024 07:29
Copy link
Contributor

@mchades mchades left a comment

Choose a reason for hiding this comment

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

LGTM.

@jerqi
Copy link
Contributor Author

jerqi commented Dec 26, 2024

@tengqm Do you have other suggestion?

@tengqm
Copy link
Contributor

tengqm commented Dec 26, 2024

@tengqm Do you have other suggestion?

LGTM, although I was thinking of a JSON type configuration file for this purpose.

@jerqi
Copy link
Contributor Author

jerqi commented Dec 26, 2024

@tengqm Do you have other suggestion?

LGTM, although I was thinking of a JSON type configuration file for this purpose.

We can expose more config options in the future. This config option is used for front end display. It's also useful to users if they use API to get the configurations.

@mchades mchades merged commit ba8df39 into apache:main Dec 26, 2024
26 checks passed
@jerqi jerqi deleted the ISSUE-5987 branch December 26, 2024 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Improvement] support 'gravitino.authorization.enable' value in '/configs' api
3 participants