-
Notifications
You must be signed in to change notification settings - Fork 3
SCANPY-102 New bootstrapping: Execute the Scan #148
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
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.
The general logic looks sound to me. The only issue regarding the logic is that the version check isn't done as the first step, but only after the JRE and scanner engine provisioning.
Apart from that, I'm not convinced that we should override __str__
for the CacheFile
and JREProvisionedPath
(see comment for more detail).
I've also left some other smaller nit picks.
cmd = self.__build_command(jre_path) | ||
print(cmd) | ||
popen = Popen(cmd, stdout=PIPE, stderr=PIPE, stdin=PIPE) | ||
outs, _ = popen.communicate(configuration.to_json().encode()) |
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.
The way this currently processes the log files means that there is no output from the scanner engine at all.
We probably should do this similarly to how it was done in the current master (see here)
However, I think it would be fine to do this as a separate ticket
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.
|
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 for the changes 😁
It seems to me that there is not really any error handling in the code. But this can be done as a separate ticket. This way we can unblock the other tickets and work on master.
SCANPY-102