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 excludes capability #342

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

hartfordfive
Copy link

Adding the exclude feature as requested in issue #89.

- Changed from pre-built file exclusion list in Prospect function to filepath.Match within the prospector scan function to ensure no race conditions could cause missed files
@@ -104,6 +105,18 @@ func (p *Prospector) scan(path string, output chan *FileEvent, resume *Prospecto
continue
}

// If the file is in the list of exclusions, skip it
for _, pattern := range exclude_paths {
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than exclude paths as a parameter maybe p.FileConfig here?

It saves the parameter list from growing :)

@driskell
Copy link
Contributor

driskell commented Jan 9, 2015

👍 Looks good I just did a tiny review to help out :)

@hartfordfive
Copy link
Author

@driskell, thanks pointing out those small improvements that can be made. I'll fix those by tomorrow and commit the changes.

…updated prospector scan method to directly use Exclude struct field from file config instead of creating an extra variable
@jordansissel
Copy link
Contributor

code looks good. can you add tests, please?

@hartfordfive
Copy link
Author

I take it you mean add the tests to the config_test.go file?

@jordansissel
Copy link
Contributor

Tests to verify that when it is configured to exclude files it behaves
correctly. Where the test lives is less important than what it tests.

I am in my phone so I can't offer specific advice on where to put the test
right now.

On Thursday, February 12, 2015, Al Lefebvre [email protected]
wrote:

I take it you mean add the tests to the config_test.go file?


Reply to this email directly or view it on GitHub
#342 (comment)
.

@hartfordfive
Copy link
Author

I apologize for the delay on this, I just haven't had the chance lately to work on this. I'll do my best figure out an appropriate testing mechanism and push it to this branch asap.

@hartfordfive
Copy link
Author

@jordansissel I've created a test suite (in the "test_vm/" directory) using test kitchen along with the shell provisioner. This test seems to do the job just fine although let me know if you have any questions or comments about it.

@webmat
Copy link

webmat commented Apr 8, 2015

The directory test_excludes should probably be deleted now.

The tests work for me. Although I need to look at bootstrap.sh carefully to understand which log files are ignored vs those that are consumed.

Perhaps you could make their names more obvious? E.g. /var/log/excluded1.log and /var/log/consumed1.log

@hartfordfive
Copy link
Author

@driskell and @jordansissel do you have any feedback about the tests I created for this feature? I figured this could also potentially be used for any other tests in the future, seeing it's self contained VM that provisions itself.

@hartfordfive
Copy link
Author

Any updates on this issue? Is there any chance you'll be merging this feature and the tests back into the master branch?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants