Fix the package not loading if it has an accidental file left in the package root directory.

[aleksmaus]

Fix the package not loading if it has an accidental file in the package root directory.
Log the error and load the remaining sibling directories.

Addresses #672

[elasticmachine]

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

• Build Cause: Pull request #673 updated

• • Start Time: 2021-01-26T13:18:22.626+0000

• Duration: 6 min 15 sec

• Commit: 8f1f28a

Test stats 🧪

Test	Results
Failed	0
Passed	169
Skipped	0
Total	169

[ruflin]

Can you add a changelog entry?

[ruflin]

I wonder if we should return an error instead. This is at first inconvenient for the developer working with the package because he has to fix it but it assures no "invalid" files are inside the package.

@mtojek @ycombinator Do we have any validation in place in elastic-package check that would detect these files?

[mtojek]

I'm wondering how did @aleksmaus run into this issue? Have you been using the elastic-package to boot up the stack? Did you interact with OSX file browser?

@ruflin This directory is not part of a package (look at the path). It's a bug in package-registry. Speaking about validation, there is such one in place, but there are places in which it's not a bug but a feature, like e.g. a package root. It's hard to detect these files as the end-user may include additional READMEs, license notes, etc.

For reference: https://github.com/elastic/package-spec/blob/master/versions/1/spec.yml#L8

@aleksmaus I don't think we need to fail the entire application in case of noticing strange files. It will be more convenient to the end-user to skip this files. Could you try this piece of code?

if info.IsDir() {
    log.Printf("%-20s\t%10s\t%s", dirs[0], dirs[1], path)
    foundPaths = append(foundPaths, path)
    return filepath.SkipDir
}
return nil

[aleksmaus]

Mac OS finder creates these pesky .DS_Store files for Finder app.
This change doesn't fail the whole app, it keeps loading the package versions.
Before the change, stumbling on such file (not dir code path) was returning filepath.SkipDir which skips all the siblings directories.
In my case the directory layout was

.DS_Store
0.1.0
0.1.2

And the existing code was stumbling upon .DS_Store file first and since it was not directory the default code path was always returning filepath.SkipDir

From documentation of filepath.Walk:

 If the function returns SkipDir when invoked on a non-directory file, Walk skips the remaining files in the containing directory.

Sure can update the code the sample you provided, it's pretty much the same what my code does.
Would still like to keep the log statement pointing at the error during load.

[aleksmaus]

@mtojek @ruflin is this good to merge now?

[ycombinator]

I would take this opportunity to make this check a bit stronger.

If the execution reaches this line, we know that we should be looking at a directory whose name is a version. So how about we not only check if info.IsDir() but also check that dirs[1] is a semver? That way we handle not only the unexpected file case but also the unexpected folder case as well.

[ycombinator]

Nit: consider changing this to a warning instead of an error and also mention that we're simply ignoring the unexpected file. That way the person looking at these logs knows this is not a severe problem to worry about.

[aleksmaus]

Updated the log message and added additional check if dir is not semver.

[aleksmaus]

[ycombinator]

LGTM. Thanks for finding and fixing this!

[ycombinator]

@aleksmaus Apologies for not catching this earlier, but would you mind making a follow up PR to add a CHANGELOG entry for the change in this PR? Thank you.

https://github.com/elastic/package-registry/blob/master/CHANGELOG.md