[Improvement] expose 'http-server.rest-auth-type' variable in Helm chart #3345
Conversation
charts/amoro/values.yaml
Outdated
| @@ -142,6 +142,7 @@ amoroConf: | |||
| ams: | |||
| adminUsername: admin | |||
| adminPassword: admin | |||
| restAuthType: token | |||
| @@ -52,6 +52,7 @@ data: | |||
| bind-port: {{ .Values.server.optimizing.port }} | |||
|
|
|||
| http-server: | |||
| rest-auth-type: {{ .Values.amoroConf.ams.restAuthType }} | |||
There was a problem hiding this comment.
it is a config in AMS REST Serivce, so rest-auth-type: {{ .Values.server.rest.restAuthType }} is better option?
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Hi @baiyangtx, please let me know if you have any questions.
I will merge this if there is no more comments.
|
Thanks for the contribution! @JiHyunSong |
Why are the changes needed?
Close #xxx.
Brief change log
How was this patch tested?
Add some test cases that check the changes thoroughly including negative and positive cases if possible
Add screenshots for manual tests if appropriate
Run test locally before making a pull request
Documentation