-
Notifications
You must be signed in to change notification settings - Fork 121
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
[elk] Add option to fetch from selected branches #650
Conversation
Signed-off-by: inishchith <inishchith@gmail.com>
cfc55ed
to
d0d1edb
Compare
Pull Request Test Coverage Report for Build 1512
💛 - Coveralls |
Thank you @inishchith for the PR, do you plan to add tests? I can have a look at the code and help you with this. |
@valeriocos I'll add the tests and ping you back. Thanks! |
@valeriocos Sorry, I'm not sure if tests could be added as we do not have tests covering the |
No worries, @inishchith I'll have a look and see if we can add some tests |
It isn't straightforward to add tests since the code should be refactored. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks @inishchith
@sduenas I tested the PR and it works fine. The change is similar to the one to include the Tests cannot be easily added because the code should be refactored. If you agree we can give it a try otherwise the PR can be accepted as it is. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Let's merge it.
Fixes: #642
@jgbarah @valeriocos Please do have a look, let me know if any changes required.
Thanks!
TODO:
Signed-off-by: inishchith inishchith@gmail.com