Skip to content
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.

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

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

Merged
merged 3 commits into from May 18, 2018

Conversation

@ncabatoff
Copy link
Collaborator

ncabatoff commented May 13, 2018

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

This comment has been minimized.

Copy link
Member

squaremo commented May 14, 2018

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

This comment has been minimized.

Copy link
Collaborator Author

ncabatoff commented May 15, 2018

Fair points. I like this approach better too.

Copy link
Member

squaremo left a comment

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!


func (c chartTracker) inChart(path string) bool {
p := path
root := fmt.Sprintf("%c", filepath.Separator)

This comment has been minimized.

Copy link
@squaremo

squaremo May 17, 2018

Member

I think we only care up to the root given to the tracker, don't we?

for _, root := range roots {
err := filepath.Walk(root, func(path string, info os.FileInfo, err error) error {
if err != nil {
return errors.Wrapf(err, "walking %q for yamels", path)
}

if info.IsDir() && looksLikeChart(path) {
if charts.isChart(path) {

This comment has been minimized.

Copy link
@squaremo

squaremo May 17, 2018

Member

isChart and inChart are a little too similar -- I can imagine typo-flips between them. How about isDirChart and isPathInChart, or similar?

@ncabatoff

This comment has been minimized.

Copy link
Collaborator Author

ncabatoff commented May 17, 2018

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

This comment has been minimized.

Copy link
Member

squaremo commented May 18, 2018

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 squaremo merged commit f9de55b into fluxcd:master May 18, 2018
@squaremo

This comment has been minimized.

Copy link
Member

squaremo commented May 18, 2018

Thanks @ncabatoff ⭐️

@ncabatoff ncabatoff deleted the ncabatoff:skip-charts-issue-1067 branch May 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.