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.

Sign up for GitHub

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

[Improvement] expose 'http-server.rest-auth-type' variable in Helm chart #3345

Merged
merged 4 commits into from
Dec 6, 2024
Merged

[Improvement] expose 'http-server.rest-auth-type' variable in Helm chart #3345

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
37d7cd1
[Improvement] expose 'http-server.rest-auth-type' variable in Helm chart
JiHyunSong Nov 27, 2024
1b40d38
Merge branch 'master' into extract-rest-auth-type
XBaith Dec 2, 2024
bf736a9
Merge branch 'master' into extract-rest-auth-type
XBaith Dec 5, 2024
c47e7ba
fix by comment
JiHyunSong Dec 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions charts/amoro/templates/amoro-configmap.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ data:
bind-port: {{ .Values.server.optimizing.port }}

http-server:
rest-auth-type: {{ .Values.amoroConf.ams.restAuthType }}
Copy link
Contributor

Choose a reason for hiding this comment

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

it is a config in AMS REST Serivce, so rest-auth-type: {{ .Values.server.rest.restAuthType }} is better option?

Copy link
Contributor Author

@JiHyunSong JiHyunSong Dec 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@XBaith
I currently see the server.rest. variables in this Helm chart as being more related to Kubernetes service or ingress configurations rather than application settings. On the other hand, I consider the configurations under amoroConf.ams. to be application-specific, which is why I placed them there. I also think this might be a part of the chart worth refactoring in the future..

Since there are differing opinions on this, I wanted to share my perspective. That said, if you still prefer moving them under server.rest, I’m happy to make the change.

Copy link
Contributor

Choose a reason for hiding this comment

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

The reason I prefer moving them under server.rest is that we already have the property HTTP_SERVER_REST_AUTH_TYPE configured as part of the HTTP REST service. It might be better to align with the original design. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @baiyangtx, please let me know if you have any questions.
I will merge this if there is no more comments.

bind-port: {{ .Values.server.rest.port }}

refresh-external-catalogs:
Expand Down
1 change: 1 addition & 0 deletions charts/amoro/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,7 @@ amoroConf:
ams:
adminUsername: admin
adminPassword: admin
restAuthType: token
Copy link
Contributor

Choose a reason for hiding this comment

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

It should under server.rest


## AMS database properties, default value is derby. For production environment, suggest to use mysql
##
Expand Down