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

Add missing assets from datasets #146

Merged
merged 5 commits into from
Nov 12, 2019
Merged

Conversation

ruflin
Copy link
Contributor

@ruflin ruflin commented Nov 11, 2019

The current code was only going 4 levels deep when looking for assets. But since we introduce datasets more level exists. This code makes sure when the package output is generated it goes into all directories.

Tests were updated with the new package format to also confirm this change.

The current code was only going 4 levels deep when looking for assets. But since we introduce datasets more level exists. This code makes sure when the package output is generated it goes into all directories.
@ruflin ruflin requested a review from skh November 11, 2019 15:36
util/package.go Outdated
return err
var assets []string
var pattern = "*"
// Iterates 6 levels deep through the tree to find assets
Copy link
Contributor

Choose a reason for hiding this comment

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

Will there be a time when 6 levels aren't deep enough? Is there a way to solve this recursively once and for all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm sure it will happen. There is a library that does it but I was hesitant to include it here: https://github.com/bmatcuk/doublestar I'm just thinking of an alternative recursive implementation now that I mention it which "should" always work. Will update.

@ruflin
Copy link
Contributor Author

ruflin commented Nov 11, 2019

@skh I just pushed a new commit to make it recursive. Let me know what you think.

@skh skh self-requested a review November 12, 2019 09:26
Copy link
Contributor

@skh skh left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the change! 👍

@ruflin ruflin merged commit 58d72ce into elastic:master Nov 12, 2019
@ruflin ruflin deleted the add-all-assets branch November 12, 2019 10:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants