Launch helper functions don't seem to work as intended(?)

catflydotio commented 2 years ago

No, this still isn't a PR. I'm reacquainting myself with it...

We know our Django launcher is greedy, claiming any project that has a requirements.txt. The files involved are flyctl/internal/sourcecode/scan.go and flyctl/internal/sourcecode/helpers.go. scan.go has the different launch configurators (Remix, Elixir, Django, etc.), and uses some helper functions in helpers.go.

It looks as though the Django launcher means to find projects with both requirements.txt and manage.py, which seems like a decent way to identify Django projects correctly, but it's using the helper function fileExists(), which returns true if either is present.

Ruby, Go, Django, and Python launchers all use fileExists() with two files. I haven't checked to see what behaviour is expected from it by the Ruby, Go, and Python launchers, but if we change the behaviour of fileExists() to fix Django, we may break something else.

I think there's a problem with `fileExists()`:

Here's fileExists() in flyctl/internal/sourcecode/helpers.go:

func fileExists(filenames ...string) checkFn {
    return func(dir string) bool {
        for _, filename := range filenames {
            info, err := os.Stat(filepath.Join(dir, filename))
            if err != nil {
                continue
            }
            if !info.IsDir() {
                return true
            }
        }
        return false
    }
}

What I think it does:

For every file in the list we pass to it,

if there's an error finding the file (it's not there), continue to the next file
if there's no error (something with that name is there) and it isn't a dir (i.e. at least one of the files exists), return true

If everything's gone through and no file was found, finally return false.

(Is it necessary to check whether the thing is a directory? Will it ever not have an extension? Putting that aside...)

What I want it to do, for the Django launcher:

For every file,

if there's an error finding the file (it's not there), return false
if there's no error but it's a dir, return false

If neither of those falses ever happen, then all files were present and we can return true.

(If we're worried there might be a dir there with the name we're looking for, what if there's both a file and a dir, and it finds the dir first? Is that possible? We'd get a false false from that...)

func fileExistsAll(filenames ...string) checkFn {
    return func(dir string) bool {
        for _, filename := range filenames {
            info, err := os.Stat(filepath.Join(dir, filename))
            if err != nil {
                return false
            }
            if info.IsDir() {
                return false
            }
        }
        return true
    }
}

(Checked on Go playground in case I was missing some subtlety around the intermediate checkFn type: https://go.dev/play/p/oc70A7a7Yl-)

I'd add this to helpers.go and rename fileExists() to fileExistsAny(), and go through the launchers to figure out which needs which.

I think there's a problem with `checksPass()`:

Next twist: fileExists() and other checks are used in a helper called checksPass().

func checksPass(sourceDir string, checks ...checkFn) bool {
    for _, check := range checks {
        if check(sourceDir) {
            return true
        }
    }
    return false
}

checksPass() takes a source directory, and potentially multiple check functions. And it returns true if any of the checks is true. It doesn't matter yet, because each time it's used so far, there's only one check. But I suspect this is not the intended behaviour for this one at all.

So I would propose renaming it to checksPassAll() and adjusting it to require all checks to pass in order to return true:

func checksPassAll(sourceDir string, checks ...checkFn) bool {
    for _, check := range checks {
        if !check(sourceDir) {
            return false
        }
    }
    return true
}

Other functions that seem OK

We also have fileContains() and dirContains(), that return true if a pattern is found at least once. Which looks like their intended behaviour. But dirContains() might be better named dirContainsPattern() or something.

Other similarly-named helpers

We've also got flyctl/helpers/fs.go, with a FileExists() and a DirectoryExists(), that each take a path and a (single) filename. FileExists() is used in 8 files, including scan.go. Contrast with fileExists() which only gets used in scan.go.

dacort commented 2 years ago

Poked at this a little bit tonight because I also ran into #878 while testing out the Flask app.

One thing I noticed about checksPass is that no uses of it actually take multiple check functions. I updated it to the following:

func checksPass(sourceDir string, check checkFn) bool {
    return check(sourceDir)
}

After changing fileExists to fileExistsAny and implementing fileExistsAll, I also went through all the scanners that used fileExists and multiple files (django, go, python, ruby). I'm fairly certain the Django one is the only one needs to make use of fileExistsAll, and even that is debatable...it could just use manage.py.

Hope that's helpful.

Attach my patch as well: fileExists.patch.txt

redjonzaci commented 1 year ago

@catflydotio can we close this?

superfly / flyctl