Fix #1067 by filtering out chart dirs from manifest loading of changed files

[ncabatoff]

I haven't written any tests yet because I first wanted to validate with the project maintainers whether this was the right approach. Another way of tackling this might be to filter out everything under the --git-charts-path option. Though in that case maybe we should also change the other manifest loading code path?

[squaremo]

Hi @ncabatoff , thanks for the PR ⭐

With respect to the approach: the Manifests interface is intended to be platform neutral, i.e., not particular to Kubernetes (notwithstanding, there are no implementations other than for Kubernetes). Charts are Kubernetes-specific concern. So, I would prefer if they were not mentioned in that interface.

There are a couple of ways this can be achieved, I think.

1. Just make sure that all methods in cluster/kubernetes that obtain Resources from files, skip charts. In other words, this bakes in the fact that files within charts shouldn't be interpreted as manifests. To my mind this is the right way to go -- the current code in master is really an incomplete implementation of this idea.

2. A fallback might be to decide on a more neutral name for LooksLikeChart -- say, ShouldBeIgnored -- and its derivatives, so that other (hypothetical) implementations of Manifests could also implement it. But then you'd have to ask "If this always gets called after loading manifests, why don't you just do it all the time in the implementation of loading manifests?" (i..e, why don't you just do 1.) above).

[ncabatoff]

Fair points. I like this approach better too.

[squaremo]

I like how you've done this ✔️
I left a couple of suggestions for you to consider; if you're are happy how it is, respond to those or leave a quick comment indicating so. Thanks!

[ncabatoff]

Thanks, I'm glad you're happy with the revised version.

Re the ambiguous method names: agreed, I had that same thought when I was writing them and then forgot to fix before pushing.

Re going up as far as the filesystem root, rather than just the chart root: I think for safety we need to check for when we reach the real filesystem root anyway. Consider what happens if the method is called with a path argument that doesn't match the root the chartTracker was constructed from; that would be a bug, I admit, but it would be a bad bug (infinite loop) if we don't have the loop terminate upon reaching the fs root. So given that we need that test anyway for safety, and we'd only be saving a few extra I/Os by also testing to see if we've reached the root the chartTracker was constructed from, and performance isn't critical here, I prefer the current simpler approach.

[squaremo]

Consider what happens if the method is called with a path argument that doesn't match the root the chartTracker was constructed from; that would be a bug, I admit, but it would be a bad bug (infinite loop) if we don't have the loop terminate upon reaching the fs root.

I suppose I am a bit nervous about / being particular to Unix-like filesystems -- not that anything in flux runs on Windows anyway!

You are right that individual path arguments aren't necessarily in the root path. They should either be supplied as relative to the root, or it should be an error to supply a path that isn't contained. I was probably a bit confused about the requirements while I wrote this, leading to neither of these being enforced. It can be the subject of a later PR, anyway.

[squaremo]

Thanks @ncabatoff ⭐