-
Notifications
You must be signed in to change notification settings - Fork 215
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
station_server: provide history handlers #936
base: master
Are you sure you want to change the base?
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
Hi Karl, Thanks! |
Now that it appears that maintainers might still exist I can look at that :) The GCLA is a bit more paperwork than the eclipse one I wasn't goign to waste time on it given the general activity level of the project earlier :) |
@googlebot I signed it! |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
@arsharma1 CLA is now signed |
with open(fn, mode="rb") as f: | ||
me = mfg_event_pb2.MfgEvent() | ||
me.ParseFromString(f.read()) | ||
from openhtf.core.test_record import TestRecord |
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.
Move imports to the top of the file.
Additionally, please do not import classes (or functions) out of modules; just import the module directly.
from openhtf.core import test_record
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.
have just dropped it, it was only needed for the type annotation
me = mfg_event_pb2.MfgEvent() | ||
me.ParseFromString(f.read()) | ||
from openhtf.core.test_record import TestRecord | ||
tr: TestRecord = mfg_event_converter.test_record_from_mfg_event(me) |
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.
PY2 is still technically supported for now, so please do not add type annotations.
I hope to drop PY2 support in the next few months as I really want annotations everywhere.
@@ -463,8 +465,25 @@ class HistoryItemHandler(BaseHistoryHandler): | |||
def get(self, file_name): | |||
# TODO(Kenadia): Implement the history item handler. The implementation |
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 remove this TODO as you're fixing it!
@@ -479,8 +498,23 @@ class HistoryAttachmentsHandler(BaseHistoryHandler): | |||
def get(self, file_name, attachment_name): | |||
# TODO(Kenadia): Implement the history item handler. The implementation |
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.
remove TODO
with open(fn, mode="rb") as f: | ||
me = mfg_event_pb2.MfgEvent() | ||
me.ParseFromString(f.read()) | ||
# TODO - could use sha1 here to check? |
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.
TODO:
This allows our TODO search tools to identify it.
|
||
fn = os.path.join(self.history_path, file_name) | ||
if not os.path.isfile(fn): | ||
self.write("Not found") |
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 use single quotes for strings.
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.
May have missed this comment.
self.write(desired_real[0].value_binary) | ||
self.set_status(200) | ||
else: | ||
self.write("some, but no match?!") |
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.
self.write('Attachment not found in test.')
d12634e
to
b79fc66
Compare
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
Updated thanks for the review |
self.set_status(404) | ||
return | ||
|
||
with open(fn, mode="rb") as f: |
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 use single quotes for strings.
self.set_status(404) | ||
return | ||
|
||
with open(fn, mode="rb") as f: |
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.
Single quotes.
|
||
fn = os.path.join(self.history_path, file_name) | ||
if not os.path.isfile(fn): | ||
self.write("Not found") |
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.
May have missed this comment.
Yes, sorry, fixed on memory and missed all your comments :( will do better. |
While the history handlers do need to match what has been saved on disk, the project provides MfgEvent protobuffer writers out of the box, and no-others, so at least let the out of box experience function as expected, even if you would want to change this in your own implementations. To enable the (built in) writers, something like this is required in your station server. ``` if __name__ == '__main__': openhtf.util.conf.load(station_server_port='4444') interface = mfg_inspector.MfgInspector() interface.set_converter(mfg_event_from_test_record) with station_server.StationServer(history_path="somepath") as server: while 1: test = .... #your tests here test.add_output_callbacks(server.publish_final_state) # explicitly match hardcoded pattern in HistoryListHandler test.add_output_callbacks(interface.save_to_disk("somepath/mfg_event_{dut_id}_{start_time_millis}.pb")) test.execute(test_start=user_input.prompt_for_test_start()) ``` Signed-off-by: Karl Palsson <[email protected]>
b79fc66
to
5ba152a
Compare
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
updated thanks! |
Anything still blocking this? |
Ok, it's approved, is there anything else to do here? |
I have applied this to my fork and tested. It works for the most part, but the MIME types of attachments are not respected. |
Seems to be fixed with the following patch: diff --git a/openhtf/output/servers/station_server.py b/openhtf/output/servers/station_server.py
index 260fbc7..50c86ee 100644
--- a/openhtf/output/servers/station_server.py
+++ b/openhtf/output/servers/station_server.py
@@ -537,6 +537,7 @@ class HistoryAttachmentsHandler(BaseHistoryHandler):
# TODO: could use sha1 here to check?
desired_real = [a for a in me.attachment if a.name == attachment_name]
if len(desired_real) > 0:
+ self.set_header('Content-Type', desired_real[0].mime_type)
self.write(desired_real[0].value_binary)
self.set_status(200)
else: |
While the history handlers do need to match what has been saved on disk,
the project provides MfgEvent protobuffer writers out of the box, and
no-others, so at least let the out of box experience function as
expected, even if you would want to change this in your own
implementations.
To enable the (built in) writers, something like this is required in
your station server.
Signed-off-by: Karl Palsson [email protected]
This change is