Skip to content

fix(plugins): reset cache repo modifications before update #31678

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

Jump to bottom
Open
OlTrenin wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix(plugins): reset cache repo modifications before update #31678

OlTrenin wants to merge 2 commits into from

Conversation

@OlTrenin
Copy link

@OlTrenin OlTrenin commented Dec 26, 2025 •
edited
Loading

Description

This PR fixes the plugin update failure issue where Helm reports "plugin repo was modified" error even when users haven't made any manual modifications to the plugin repository.

Closes #31664

What this PR does / why we need it:

During plugin installation, Helm modifies the plugin.yaml file in the cached git repository (located at $HOME/.local/share/helm/cache/plugins/). When users attempt to update the plugin later, the Update() function checks if the repository is dirty and returns an error if any modifications are detected. This prevents plugin updates from succeeding.

This PR automatically resets local modifications in the cached repository before attempting to update. Since the cached repository is managed entirely by Helm and should not contain user modifications, this is a safe operation that allows updates to proceed successfully.

Changes:

  • Added resetRepo() function that discards local modifications using VCS-specific commands (git reset, hg revert, etc.)
  • Modified Update() to automatically reset the cache repository if it's dirty before pulling updates
  • Added comprehensive tests to verify the reset functionality works correctly

Special notes for your reviewer:

  • The fix only affects the cached repository (~/.local/share/helm/cache/plugins/), not the actual plugin directory (~/.local/share/helm/plugins/) where users might make local modifications
  • The resetRepo() function supports all VCS types that Helm supports: Git, Mercurial, Bazaar, and SVN
  • The original dirty check was added in commit 1e8ebae to provide better error messages, but it didn't account for Helm's own modifications during installation
  • This is a safe change because the cache directory is an internal implementation detail managed solely by Helm

If applicable:

  • this PR contains user facing changes (the docs needed label should be applied if so)
    • This is a bug fix that makes updates work as expected - no documentation changes needed
  • this PR contains unit tests
    • Added TestResetRepo() to test the reset functionality
    • Updated TestVCSInstallerUpdate() to verify automatic reset during updates
  • this PR has been tested for backwards compatibility
    • The change only affects error cases that previously failed, making them succeed
    • No API changes, no breaking changes to existing functionality

Testing:

The fix was tested with:

  1. Unit tests that simulate the exact scenario from issue Helm plugin update fails #31664
  2. All existing plugin installer tests pass
  3. The new TestResetRepo() verifies git reset works correctly
  4. The updated TestVCSInstallerUpdate() confirms updates succeed after modifications

Reproducing the original issue (before this PR):

helm plugin install https://github.com/helm-unittest/helm-unittest.git
helm plugin update unittest
# Error: failed to update plugin unittest, got error (plugin repo was modified)

After this PR:
helm plugin install https://github.com/helm-unittest/helm-unittest.git
helm plugin update unittest

@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 26, 2025
TerryHowe
TerryHowe reviewed Jan 3, 2026
View reviewed changes
internal/plugin/installer/vcs_installer.go
func resetRepo(repo vcs.Repo) error {
// Check the VCS type to determine the appropriate reset command
switch repo.Vcs() {
case vcs.Git:
Copy link
Contributor

@TerryHowe TerryHowe Jan 3, 2026

Choose a reason for hiding this comment

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

I can see how this will work, but ideally, vcs should add this I would think.

Copy link
Contributor

@TerryHowe TerryHowe Jan 3, 2026

Choose a reason for hiding this comment

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

Also, if plugin.yaml is the only problem, maybe just reset that one file.

Copy link
Author

@OlTrenin OlTrenin Jan 4, 2026

Choose a reason for hiding this comment

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

@TerryHowe hi, okey, I have improved by your comment's 😉

@benoittgt
Copy link
Contributor

benoittgt commented Jan 13, 2026

I tested the change locally, and it does fix the failure. I’m not sure about resetting the repository during an update, but it seems like the simplest approach. I’m a bit worried we might miss something when resetting this cache folder.

I’m also wondering whether the reset logic should be extracted into a separate function so we can reuse it.

@OlTrenin
Copy link
Author

OlTrenin commented Jan 13, 2026

I tested the change locally, and it does fix the failure. I’m not sure about resetting the repository during an update, but it seems like the simplest approach. I’m a bit worried we might miss something when resetting this cache folder.

I’m also wondering whether the reset logic should be extracted into a separate function so we can reuse it.

Thanks for the review! The concerns you raised have already been addressed in the second commit (98598c6):

  1. We now reset only plugin.yaml, not the entire repository
  2. The reset logic is already extracted into a separate resetPluginYaml() function
  3. plugin.yaml is the only file Helm modifies during installation, so there's nothing else to worry about missing

The cache folder is managed by Helm internally, so resetting this specific file is safe

@benoittgt
Copy link
Contributor

benoittgt commented Jan 13, 2026

My concerns stem from your last proposal. I’m wondering whether we still want to keep the change in 1e8ebae. Another option could be to stop checking the dirty state altogether. 🤷

The reset logic is already extracted into a separate resetPluginYaml() function

I was thinking about extracting the reset mechanism itself into a generic function that accepts a path and the VCS. So function does less and that way, we could easily reuse it to reset other files in other locations. That said, this might be premature optimization.

@OlTrenin
Copy link
Author

OlTrenin commented Jan 13, 2026

I was thinking about extracting the reset mechanism itself into a generic function that accepts a path and the VCS. So function does less and that way, we could easily reuse it to reset other files in other locations. That said, this might be premature optimization.

Good point, but I agree it's premature optimization for now. We can always generalize it later if we need to reset other files. Keeping it simple for this PR or should I implement it?

@benoittgt
Copy link
Contributor

benoittgt commented Jan 14, 2026

I'm curious about what @gjenkins8 is thinking about this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TerryHowe TerryHowe TerryHowe left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Helm plugin update fails

3 participants

@OlTrenin @benoittgt @TerryHowe