feat: Added multi-platform plugin hook support

[stevehipwell]

What this PR does / why we need it:

This PR addresses #7117 to allow plugin installation on Windows in a backwards compatible way. It also adds better OS specific hook support and improved platform command logic to support args containing a space.

I've forked helm-diff at https://github.com/stevehipwell/helm-diff to provide an example of how this works,

Special notes for your reviewer:

• Closes #7117
• Docs PR https://github.com/helm/helm-www/pull/1627

If applicable:

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

[stevehipwell]

@sabre1041 could you or one of the other maintainers please take a look at this?

[sabre1041]

@sabre1041 could you or one of the other maintainers please take a look at this?

I've started to take a look. Will add some further thoughts in the next day or so

[stevehipwell]

@sabre1041 have you been able to find any time to look at this? And if so is there any feedback?

[sabre1041]

@sabre1041 have you been able to find any time to look at this? And if so is there any feedback?

getting back up to speed after a work conference. will review this week

[sabre1041]

@stevehipwell Receiving the following error on windows when installing a plugin:

helm plugin install https://github.com/databus23/helm-diff
/usr/bin/bash: C:Users<USERNAME>AppDataRoaminghelmpluginshelm-diff/install-binary.sh: No such file or directory
Error: plugin install hook for "diff" exited with error

Not receiving the same error with the latest helm release

[stevehipwell]

@sabre1041 the error should be consistent, but not necessarily identical, to the existing behaviour on Windows. Basically no currently published Helm plugins can support Windows (unless they have no hooks) so the install script should still fail with the new code.

[sabre1041]

@sabre1041 the error should be consistent, but not necessarily identical, to the existing behaviour on Windows. Basically no currently published Helm plugins can support Windows (unless they have no hooks) so the install script should still fail with the new code.

Contents of PR causes an error while the released code completes successfully. Tested using the helm diff plugin

[stevehipwell]

@sabre1041 I'm getting the exact same error for both the published code and the code on this branch using my Windows machine.

helm plugin install https://github.com/databus23/helm-diff
Error: exec: "sh": executable file not found in %PATH%

Has the Windows machine you're currently testing on been configured to support sh and do you have a custom value for HELM_PLUGIN_DIR set in this non-Windows shell? If so the difference could be caused by the early evaluation of the environment variables in the new version, which matches the way the platform command functions. I could short circuit the behaviour so that the legacy behaviour stays consistent.

[stevehipwell]

@sabre1041 this should now keep the current behaviour on Windows machines which can run sh commands.

[stevehipwell]

@sabre1041 have you had a chance to look at this again since I made the changes to fix the legacy behaviour on a Windows machine with Bash support?

[sabre1041]

@stevehipwell shoot, I did review it after you made the prior change and the good news, it works as expected. I'll try to have another maintainer review as well (finding individuals with access to Windows machines could be a challenge)

[sabre1041]

@stevehipwell functionality works. However, I am also receiving the same unit test failures that are being reported by the GH run. Are you able to take a look?

[stevehipwell]

@sabre1041 I've just re-run all of the tests I added or touched and they run fine locally. I've just rebased to see if that was the issue. Could you please re-run the workflow?

[sabre1041]

@stevehipwell Still seeing the same issues

[stevehipwell]

@sabre1041 the issue is due to inconsistent behaviour between the way legacy plugin hooks and legacy plugin commands work. I've updated the code and the tests should pass again now. Could you please re-run them.

[stevehipwell]

@sabre1041 sorry forgot to push the test changes, could you please re-run again.

[stevehipwell]

@sabre1041 I've ended up re-writing all of the tests for the plugin prepare logic to be consistent and clear.

The failure on the last version was related to the existing behaviour of taking a mismatching architecture command and using it if there are no full matches. I've removed the test to match the behaviour change I implemented, which doesn't support attempting to use a command for the wrong architecture. I can't see how this is desired behaviour as if the commands are platform specific and the architecture is set this should error. I can see a value to adding support for leaving arch and os empty to allow falling back outside of the legacy command value; I haven't implemented this but I could.

[stevehipwell]

@sabre1041 could you point me at instructions for the process to make the corresponding docs update?

[sabre1041]

@sabre1041 could you point me at instructions for the process to make the corresponding docs update?

Docs are found in this.

Doc specific to plugins is here

[stevehipwell]

Thanks @sabre1041 I was more referring to the process of updating the docs in sync with code changes?

[sabre1041]

@sabre1041 could you point me at instructions for the process to make the corresponding docs update?

Create a PR against the docs repo and include the reference within this PR

[Xitric]

Hey @sabre1041, what is the ETA for having this merged? You mentioned looking for another maintainer to review the code.

Our DX team could really benefit from using Helm plugins, but since all of our developers work from Windows platforms, support for hooks on Windows is quite important for us.

[stevehipwell]

@Xitric I need to add another change to this PR and create a docs PR before it can be re-reviewed. I'm hopeful I can get this done today.

[stevehipwell]

@sabre1041 this PR is ready for a re-review and I've created the corresponding docs PR (https://github.com/helm/helm-www/pull/1627).

[stevehipwell]

The above error looks to be caused by using old code to access my fork of Helm Dif?

[sabre1041]

The above error looks to be caused by using old code to access my fork of Helm Dif?

Indeed I was testing against your fork to verify the configuration. Is it no longer a valid use case?

[TBBle]

The fork relies on the updated helm from this PR. Unlike the helm-diff fork, I expect most updates will keep the existing command stanza for back-compat reasons, and add a platformCommand block for updated users, i.e. the hooks block will remain and then a platfomHooks block would be added for Windows. Although if existing helm rejects the new platformHooks block, then maybe plugin authors can't actually do that?

I also note that in the helm-diff fork, PlatformCommand is capitalised, but probably shouldn't be? Same for PlatformHooks in the test-case and function comment here. The Go code correctly specified "platformHooks" so I'm mildly surprised this works. YAML is case-sensitive, but maybe the Go yaml parser is not?

Anyway, if possible, I think it might be worth having a test-case for platformHooks "no-os" and confirm the expected fallback behaviour to the hooks block, even though it's marked to be removed in Helm 4.

[stevehipwell]

@sabre1041 I created the fork of Helm Diff to only be compatible with the new code, the existing version of Helm Diff should work fine with the new code for currently supported platforms. This was an explicit breaking change for testing purposes, but I wasn't expecting an error about unmarshalling the YAML as the plugin spec has already had platformCommand added. I was expecting the new fields to be ignored, is this not the intended behaviour?

@TBBle thanks for spotting the incorrect capitalization, I found one previously and was also confused as to why it worked; these should now be fixed. There is a test in this PR (TestPrepareCommandsFallback) to check that the fallback logic works correctly.

[TBBle]

I was expecting the new fields to be ignored, is this not the intended behaviour?

Sadly, looks like we use UnmarshalStrict so that'll fail parsing if it sees unknown fields.

So there's no way to add fields that isn't a back-compat break, that I can immediately think of.

[stevehipwell]

Given that the Helm plugin manifest doesn't follow KRM semantics like the Helm chart manifest and doesn't already support a helmVersion field (comparable to the Helm chart kubernetesVersion field) I'm don't think there is good solution for this.

Longer term I think the following actions should be considered to augment Helm plugins.

• Switch to Unmarshal
• Add an apiVersion field for the plugin schema to support breaking changes
• Add a helmVersion or helmMinVersion field to support minimum version of Helm required to use the plugin