-
Notifications
You must be signed in to change notification settings - Fork 25
Validate conf files in the worker container #247
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
base: master
Are you sure you want to change the base?
Conversation
Just a small improvement in order to validate what is mount in the container as the `wait_for_container_log` confirms that it cannot find any keys but we do not know whether the configs which reads are as expected. related poo: https://progress.opensuse.org/issues/186651 Signed-off-by: Ioannis Bonatakis <[email protected]>
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.
so you're basically just checking whether the volume option in docker works correctly?
my $data_container_path = '/root/openQA/container/openqa_data/data.template/'; | ||
$confs{client_conf}{output} = script_output("cat $data_container_path$confs{client_conf}{path}"); | ||
$confs{worker_ini}{output} = script_output("cat $data_container_path$confs{worker_ini}{path}"); | ||
|
||
assert_script_run("docker run -d --network testing $volumes --name openqa_worker openqa_worker"); | ||
wait_for_container_log('openqa_worker', 'API key and secret are needed', 'docker'); |
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.
Why aren't you addressing this error message? If that's gone we would know config was ready and the connection works, no?
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.
Addressing means? If you mean get a registred worker, that it would imply more changes, as it needs to be clear (at least to me) what is the intention. As I mentioned, I think the message (API key and secret are needed
) here is the expected one.
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.
Right now the worker test prepares a container and validates that it is not able to connect to a web UI.
If you're saying we should test both this, and the case of a successful connection I would say, yes, that makes sense.
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.
So we want to redesign the modules?
Does it mean that it will be better a multimachine test?
in any case this PR is just an additional check. it wasnt intented to change how things are now.
Ensure that the |
Just a small improvement in order to validate what is mount in the container as the
wait_for_container_log
confirms that it cannot find any keys but we do not know whether the configs which reads are as expected.related poo: https://progress.opensuse.org/issues/186651