Skip to content

Conversation

sureshdsk
Copy link
Contributor

@sureshdsk sureshdsk commented Apr 29, 2022

Signed-off-by: Suresh Kumar sureshdsk91@gmail.com

What this PR does / why we need it:
closes #10884
bug: plugin does not work when HELM_DATA_HOME directory path contains spaces
solution:
existing method expands env variable and then splits command sequence by space. if the path contains space, results in error. so expanding just before executing the command fixes the issue.

If applicable:

  • this PR contains documentation
  • this PR contains unit tests
  • this PR has been tested for backwards compatibility

@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Apr 29, 2022
@jkroepke
Copy link

Did you forget

parts = strings.Split(os.ExpandEnv(p.Metadata.Command), " ")

?

I guess, someone would ask for unit tests here.

@sureshdsk sureshdsk force-pushed the pluginbug branch 2 times, most recently from d3d9cda to 813288d Compare April 29, 2022 10:52
Signed-off-by: Suresh Kumar <sureshdsk91@gmail.com>
@pull-request-size pull-request-size bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Apr 29, 2022
@yxxhero
Copy link
Member

yxxhero commented May 1, 2022

@sureshdsk Thanks very much. need some unittest for this pr.

Signed-off-by: Suresh Kumar <sureshdsk91@gmail.com>
@pull-request-size pull-request-size bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 1, 2022
@sureshdsk
Copy link
Contributor Author

sureshdsk commented May 1, 2022

@yxxhero I've added tests. can you take a look?

@sureshdsk
Copy link
Contributor Author

@yxxhero its been a while. just following up. do you want review it or assign the code review to someone?

Copy link
Contributor

@joejulian joejulian left a comment

Choose a reason for hiding this comment

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

This looks good to me, but I'm not a maintianer.

@joejulian joejulian added this to the 3.10.2 milestone Nov 1, 2022
@mattfarina mattfarina modified the milestones: 3.10.2, 3.10.3 Nov 10, 2022
@hickeyma hickeyma modified the milestones: 3.10.3, 3.11.0 Dec 14, 2022
@mattfarina mattfarina modified the milestones: 3.11.0, 3.12.0 Jan 18, 2023
@joejulian joejulian modified the milestones: 3.12.0, 3.13.0 May 5, 2023
@joejulian joejulian added Has One Approval This PR has one approval. It still needs a second approval to be merged. and removed awaiting review labels Aug 1, 2023
@mattfarina mattfarina merged commit dbef83e into helm:main Sep 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Has One Approval This PR has one approval. It still needs a second approval to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

If $HELM_DATA_HOME contains spaces, plugins stop working
6 participants