Skip to content
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

Improve is 05 control #701

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

pkeroulas
Copy link

This is a attempts to ease the testing process.

Copy link
Contributor

@garethsb garethsb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick review, haven't run it, sorry!

Comment on lines +32 to +33
--request REQUEST JSON data to be sent in the request to configure sender
--sdp SDP SDP file to be queried from a Sender (write) or be sent to a Receiver (read)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Explanation of --sdp is a bit hard to understand... maybe:

Suggested change
--request REQUEST JSON data to be sent in the request to configure sender
--sdp SDP SDP file to be queried from a Sender (write) or be sent to a Receiver (read)
--request REQUEST Input JSON file for PATCH /staged request to the Sender
--sdp SDP Input SDP file for PATCH /staged request to the Receiver, or
Output SDP file for GET /transportfile response from the Sender

Press 'c' to set a valid config on a Sender (JSON) or a Receiver(SDP)
Press 'u' to set a dummy config on a Sender (JSON) or a Receiver(SDP)
Press '7' to set 2022-7 Sender to dummy config
Press 's' to get SDP file (and save to "./new.json" from a Sender)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does it say save to ./new.json? That's a weird file extension for SDP! Ah, it's actually the name used in the --sdp arg or latest.sdp by default? I guess when you grabbed this text from a run you had passed --sdp new.json?! How about saying "./<SDP>" or something here?

Another thought... Why is saving only supported for Senders? Hmm, I suppose because you don't want to overwrite an SDP file provided via --sdp on the command line?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new.json is indeed a mistake.

Right, It makes sense to save from a Sender but just display the patched SDP from a receiver.

Comment on lines 41 to 42
Press 'c' to set a valid config on a Sender (JSON) or a Receiver(SDP)
Press 'u' to set a dummy config on a Sender (JSON) or a Receiver(SDP)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why it's important to indicate file type, for these one character commands that have no parameters that the user can specify? I think the important point is that the "valid" config means the config specified via --request or --sdp on the command line, right?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, let's not over-complicate the helper msg.

Comment on lines +56 to +58
2. Enable (`e`)
3. Push a valid RTP config (`c`)
4. Save SDP file (`s`)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trivial nit: seems a bit inconsistent to use parentheses and backticks for the commands here but single quote marks above?

@@ -113,21 +113,41 @@ def configure_receiver(url, sender_id, sdp_data):

send_request(url, body)

def get_request(url):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function naming seems confusing.

I think send_request is "<verb>_<noun>"
but get_request is "<adjective>_<noun>"

I think spelled out, they could be more clearly send_patch_request and send_get_request?

Comment on lines 158 to +159
parser.add_argument("--request", help="JSON data to be sent in the request to configure sender")
parser.add_argument("--sdp", help="SDP file to be sent in the request to configure receiver")
parser.add_argument("--sdp", help="SDP file to be queried from a Sender (write) or be sent to a Receiver (read)")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Comment in the README.md)

Comment on lines 191 to 210
print('Press \'c\' to set Sender or Receiver to valid config')
print('Press \'u\' to set Sender or Receiver to dummy config')
print('Press \'c\' to set a valid config on a Sender (JSON) or a Receiver(SDP)')
print('Press \'u\' to set a dummy config on a Sender (JSON) or a Receiver(SDP)')
print('Press \'7\' to set 2022-7 Sender to dummy config')
print('Press \'s\' to get SDP file (and save to "./{}" from a Sender)'.format(sdp_filename))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Comment in the README.md)

else:
configure_receiver(url, "xxxxxxxx-1dd2-xxxx-8044-cc988b8696a2", dummy_sdp_payload)
configure_receiver(url_staged, "aaaaaaaa-1dd2-11b2-8044-cc988b8696a2", dummy_sdp_payload)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, it was expecting that no-one validated the sender_id! Good change!

Comment on lines 199 to 202
if not os.path.exists(sdp_filename):
with open(sdp_filename, "w") as sdp_file:
sdp_file.write("")
print("Create empty SDP file \"{}\"".format(sdp_filename))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this necessary?

* Now we can get SDP from a sender (`--sdp` is now re-used to save
transport file) and a receiver. To acheive this a GET request is added
which implies some renaming.

* The http/json ack is now displayed for all the requests

* Update the doc including a practical example
The former uuid wasn't accepted by a receiver because it didn't match:
'^[0-9a-f]{8}-[0-9a-f]{4}-[1-5][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}'
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants