-
Notifications
You must be signed in to change notification settings - Fork 90
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
MINIFICPP-1048 - Add PublishKafka docker tests #657
Conversation
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.
Useful stuff nicely done, thanks!
Added some comments, the interface change of validators is definitely mandatory one, otherwise I'm happy with this.
Some more tests scenarios should be added as a follow-up, but that's fine in scope of another ticket.
@bakaid surely has some ideas
|
||
if self.valid: | ||
return True | ||
# TODO: Why did we do this btw? |
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.
To avoid validating sg. twice.
In case a validator already passed, we are happy.
Not sure if that really occurs or not, I guess not, so I don't mind this to be removed.
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.
Will remove it, since I run validate many times with different dirs
@@ -213,24 +245,75 @@ def __init__(self, expected_content): | |||
self.valid = False | |||
self.expected_content = expected_content | |||
|
|||
def validate(self): | |||
def validate(self, dir=''): |
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.
Please don't change the signature here.
FileOutputValidator is the base class, where it makes more sense.
>> (('success', PutFile('/tmp/output/success')), | ||
('failure', PutFile('/tmp/output/failure')))) | ||
|
||
with DockerTestCluster(SingleFileOutputValidator('test')) as cluster: |
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.
I really like this, thanks!
0e86f6b
to
96bfe07
Compare
self.network = self.client.networks.create(net_name) | ||
# Set IP | ||
ipam_pool = docker.types.IPAMPool( | ||
subnet='192.168.42.0/24', |
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.
Do you think it's going to work with whatever docker config developers may have?
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.
Getting there, some minor comments.
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, nice work!
Will merge
Signed-off-by: Arpad Boda <aboda@apache.org> This closes apache#657
Thank you for submitting a contribution to Apache NiFi - MiNiFi C++.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
For all changes:
Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
Does your PR title start with MINIFICPP-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
Has your PR been rebased against the latest commit within the target branch (typically master)?
Is your initial contribution a single, squashed commit?
For code changes:
For documentation related changes:
Note:
Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.