Skip to content

Conversation

foot
Copy link
Collaborator

@foot foot commented Nov 3, 2022

  • Refactor to support testing a bit better
  • Clean up logging

Closes #1823

What changed?

We remove the watcher from our store if the clusterManager sends an update.Remove

Why was this change made?

It was overlooked / undertested when intro'd in #1631

How was this change implemented?

Bit of refactoring and adding the important line to remove the watcher from the store.

How did you validate the change?

More unit tests

- Refactor to support testing a bit better
- Clean up logging
@foot foot changed the title Actually remove the watcher from the cache Actually remove the watcher from the helm-watcher-cache Nov 3, 2022
@foot foot added the bug Something isn't working label Nov 4, 2022
@foot foot marked this pull request as ready for review November 4, 2022 08:00
@foot foot requested review from sarataha and bigkevmcd November 4, 2022 08:00
Copy link
Contributor

@bigkevmcd bigkevmcd left a comment

Choose a reason for hiding this comment

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

Looks good to me, lots of code for that single line, but I appreciate the good tests.

@@ -55,6 +64,8 @@ func (i *ClusterHelmIndexerTracker) Start(ctx context.Context, cm clustersmngr.C
watcher, ok := i.ClusterWatchers[removed.Name]
if ok {
watcher.Stop()
// remove the watcher from the map
delete(i.ClusterWatchers, removed.Name)
Copy link
Contributor

Choose a reason for hiding this comment

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

A big branch for this :-)

foot and others added 2 commits November 4, 2022 18:56
- Combine the fake caches
- Clean up error handling
@foot foot merged commit 34c213b into main Nov 7, 2022
@foot foot deleted the fixes-removing-clusters branch November 7, 2022 08:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add-app doesn't list helmrepositories when re-using cluster names
2 participants