Http url output support #2
Conversation
CaitlinOCallaghan
requested review from
colleenkhenry,
ngarg17 and
stephenlindauer
streamer/controller_node.py
Outdated
| if os.path.exists(self._output_dir): | ||
| shutil.rmtree(self._output_dir) | ||
| os.mkdir(self._output_dir) | ||
| CloudNode.check_access(bucket_url) |
There was a problem hiding this comment.
style nit: Please remove the trailing whitespace
streamer/controller_node.py
Outdated
| @@ -24,6 +24,7 @@ | |||
| import os | |||
| import re | |||
| import shutil | |||
| import streamer.util | |||
There was a problem hiding this comment.
Style nit: Move this down to the other stream imports below
|
|
||
| if bucket_url: | ||
| raise RuntimeError( | ||
| 'Cloud bucket upload is incompatible with HTTP PUT support.') |
There was a problem hiding this comment.
You have my permission to kill CloudNode once we have a built-in auth proxy that can handle GCS. We're currently using CloudNode internally for this stream:
There was a problem hiding this comment.
Woohoo! I look forward to the day when we can kill it. :D
| else : | ||
| # Check some restrictions and other details on HTTP output. | ||
| if not self._pipeline_config.segment_per_file: | ||
| raise RuntimeError( |
There was a problem hiding this comment.
Do we wish to use a more specific error like ConflictingFields or MalformedField, found in configuration.py? I feel that these errors are more for configuration inputs though, and the output is its own entity rather than a configuration. We can also create a new error type if Runtime Error does not suffice?
There was a problem hiding this comment.
We're using RuntimeError in a few similar places, so I think this is okay for now. I'd be fine with a new error type, if you like, and it may become necessary or useful to have one in the future. But I'm not worried about it at the moment.
tests/tests.js
Outdated
| }; | ||
|
|
||
| await expectAsync(startStreamer(inputConfig, pipelineConfig, {}, outputHttpUrl)) | ||
| .toBeRejected(); |
There was a problem hiding this comment.
I tried using .toBeRejectedWithError() and .toBeRejectedWith(ErrorType), but was unable to get the tests to pass. @joeyparrish, do you have JavaScript testing expertise to share regarding this matter? I think I am missing something here.
There was a problem hiding this comment.
Error types in Python are translated into JSON fields by run_end_to_end_tests.py:
if isinstance(e, ConfigError):
body = json.dumps({
'error_type': type(e).__name__,
'class_name': e.class_name,
'field_name': e.field_name,
'field_type': e.field.get_type_name(),
'message': str(e),
})
return createCrossOriginResponse(
status=418, mimetype='application/json', body=body)But it appears to only catch ConfigError subtypes. You can add a check for RuntimeError there and a similar format for those.
There was a problem hiding this comment.
Thank you for pointing this out! I didn't realize that this was in-house; I had the misinterpretation that the test framework did the parsing and mapping. I will add a RuntimeError check to run_end_to_end_tests.py.
tests/tests.js
Outdated
| segment_per_file: false, | ||
| }; | ||
|
|
||
| await expectAsync(startStreamer(inputConfig, pipelineConfig, {}, outputHttpUrl, cloudUrl)) |
There was a problem hiding this comment.
This call actually fails on the cloud_url authentication check rather the check for a HTTP output and cloud storage see here. Is there a test gs bucket or some sort of gs dev/null/ to use as a test cloud url input? If not, then I can create a mock service that lets the cloudUrl pass the validity checks.
There was a problem hiding this comment.
I would suggest just deleting this test. We're killing cloud node eventually, so leaving this conflict untested in the meantime is fine with me. I certainly don't think it's worth the time and effort to mock it, given that we are transitioning away from it.
There was a problem hiding this comment.
Okay, sounds great!
tests/tests.js
Outdated
| inputConfig.multiperiod_inputs_list = [ | ||
| getBasicInputConfig(), | ||
| const inputConfig = { | ||
| 'multiperiod_inputs_list': [ | ||
| getBasicInputConfig(), |
## Feature - Allow users to specify a HTTP url as an output - Segments and manifests will be uploaded to the specified url via HTTP PUT - Urls ending with a '/' will have the slash removed to prevent issues with Google Cloud ## Testing - Added unit tests to check for config compatibility errors, such as specifying an HTTP url output with a "multi_period_input" and with "segment_per_file" set to false ## Shortcomings - The multi period input list feature is currently structured to use the local file system and does not easily support HTTP uploads. This must be addressed as the multi period input feature continues to evolve - The HTTP upload feature does not make sense if used with a cloud url, and although there is an error check for this, there is no unit test. The cloud url unit test would have required a mock bucket to pass the cloud url validity checks. Because of the work involved in writing a mock bucket and [the future (and hopefully soon) removal of the CloudNode](CaitlinOCallaghan#2 (comment)), it was decided that forgoing this particular unit test was okay.
|
Merged into Google's main branch! 🥳 |
Based off of Joey Parrish's HTTP Upload commit
Feature
Testing
Shortcomings