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

[K8S][HELM] Add command and args configuration support #4606

Closed
wants to merge 1 commit into from

Conversation

dnskr
Copy link
Contributor

@dnskr dnskr commented Mar 25, 2023

Why are the changes needed?

The change allows to change default command to launch Kyuubi and pass additional command line arguments.
It's needed if the chart user wants to print message, create files or similar in main container (kyuubi-server) when pod starts.

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 make a pull request

@codecov-commenter
Copy link

Codecov Report

Merging #4606 (5e0ba5b) into master (cee2f00) will decrease coverage by 0.05%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master    #4606      +/-   ##
============================================
- Coverage     53.32%   53.28%   -0.05%     
  Complexity       13       13              
============================================
  Files           577      577              
  Lines         31584    31584              
  Branches       4248     4248              
============================================
- Hits          16843    16830      -13     
- Misses        13154    13165      +11     
- Partials       1587     1589       +2     

see 8 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@pan3793
Copy link
Member

pan3793 commented Mar 26, 2023

I'm not sure it's a good practice to allow user customize the start command, any examples?

@dnskr
Copy link
Contributor Author

dnskr commented Mar 26, 2023

I'm not sure it's a good practice to allow user customize the start command, any examples?

I think it is not a good practice for production usage, but very helpful for investigation, development and workarounds.

For example

So it is not critical change, however gives more flexibility to the helm chart.

@pan3793
Copy link
Member

pan3793 commented Mar 26, 2023

Thanks @dnskr for listing usage examples, it makes sense then.

@pan3793 pan3793 added this to the v1.7.1 milestone Mar 26, 2023
@pan3793 pan3793 closed this in 5c90e84 Mar 26, 2023
pan3793 pushed a commit that referenced this pull request Mar 26, 2023
### _Why are the changes needed?_
The change allows to change default command to launch Kyuubi and pass additional command line arguments.
It's needed if the chart user wants to print message, create files or similar in main container (`kyuubi-server`) when pod starts.

### _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](https://kyuubi.readthedocs.io/en/master/develop_tools/testing.html#running-tests) locally before make a pull request

Closes #4606 from dnskr/add_command_and_args.

Closes #4606

5e0ba5b [dnskr] [K8S][HELM] Add command and args configuration support

Authored-by: dnskr <dnskrv88@gmail.com>
Signed-off-by: Cheng Pan <chengpan@apache.org>
(cherry picked from commit 5c90e84)
Signed-off-by: Cheng Pan <chengpan@apache.org>
@dnskr dnskr deleted the add_command_and_args branch May 11, 2023 20:42
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.

3 participants