Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Refactor: unify event updates to happen in one place #297

Merged
merged 4 commits into from
Jun 9, 2021

Conversation

kares
Copy link
Contributor

@kares kares commented Jun 9, 2021

This is a simple refactoring to have event.set in one place for easy understanding.

There's also 2 minor changes to make CI more stable - properly re-trying on rspec expectation failure

on `RSpec::Expectations::ExpectationNotMetError`
@kares kares marked this pull request as ready for review June 9, 2021 14:04
@kares kares changed the title Refactor: unify post_process event sets Refactor: unify event updates to happen in one place Jun 9, 2021
@kares kares requested a review from robbavey June 9, 2021 14:04
Copy link
Contributor

@robbavey robbavey left a comment

Choose a reason for hiding this comment

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

Minor question/nit.

@@ -363,7 +363,9 @@ def wait_for_start_processing(run_thread, timeout: 1.0)
end
end

def wait_for_file_removal(path, timeout: 3 * interval)
wait(timeout).for { File.exist?(path) }.to be_falsey
def wait_for_file_removal(path, timeout: interval)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see the timeout parameter used anywhere, can we remove it?

Copy link
Contributor

@robbavey robbavey left a comment

Choose a reason for hiding this comment

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

LGTM

@kares kares merged commit 3624548 into logstash-plugins:master Jun 9, 2021
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants