TIMOB-8921: Blackberry: Add functionality to specify ip address (device/simulator) and (optionally) password #67
Conversation
|
|
||
| parser.add_argument('command', choices=['build', 'run'], help='commands') | ||
| parser.add_argument('-t', '--type', choices=['simulator', 'device', 'deploy'], help='simulator | device | deploy', required=True) | ||
| parser.add_argument('-ip', '--ip_address', help='(simulator | device) ip address', required=True) | ||
| parser.add_argument('-pw', '--password', help='(simulator | device) protection password') |
There was a problem hiding this comment.
the jira says to use --device_password
There was a problem hiding this comment.
The convention with short options is that they are one character long. Do not create short options with more than one character. It is not mandatory to have short options, we can stick to only long option if no single letter makes sense for a short option
There was a problem hiding this comment.
Password is used also for simulator, so --password will be more correct.
Short options updated.
There was a problem hiding this comment.
The studio team is coding to the spec in jira, so we can't just change it.
|
Reviewed |
Have you tested both with and without password? Also what happens if password is required but not given? Have you tested deploying to both simulator and device? It is not clear from the wording of the tests that all these situations were tested. |
| 'device' : ('o.le-v7-g', 'arm'), | ||
| 'deploy' : ('o.le-v7', 'arm')} | ||
|
|
||
| def __init__(self, project_dir, type, ndk): | ||
| def __init__(self, project_dir, type, ipAddress, ndk, password = None): |
There was a problem hiding this comment.
Do not change the ordering of params in existing functions, ipAddress should come after ndk
|
Reviewed |
|
Add the url to this PR in the Jira ticket |
|
Updated |
|
Update missed comments |
| @@ -99,11 +111,21 @@ def error(msg): | |||
| print >>sys.stderr, e | |||
| sys.exit(1) | |||
|
|
|||
| builder = Builder(args.project_path.decode('utf-8'), args.type.decode('utf-8'), bbndk) | |||
| if (args.subparser_name == 'run'): | |||
| # Validate ip address | |||
There was a problem hiding this comment.
The only problem I have with us validating the IP is that this won't allow hostnames, but the bbndk does. Then again if we're calling the parameter ip_address then we're being fairly specific. This would be something to discuss with Appcelerator in case they would like us to support hostnames. Shouldn't hold back this patch though.
There was a problem hiding this comment.
blackberry-deploy supports only ip address, so probably if we decide to do that, needs to be resolve the ip for given hostname. Probably no need to do it right now.
There was a problem hiding this comment.
Where do you see that blackberry-deploy is only ip address? I'm looking at the usage output:
<device> - hostname or the IP address of the target device or simulator
plus I've tried it. It spits out an error if you give it a bad ip or if it can't resolve a hostname, but it works fine with a resolvable hostname.
There was a problem hiding this comment.
David, can you set the hostname of a device? Otherwise i'm not sure what the usefulness would be to support hostname
There was a problem hiding this comment.
I think we should support it because bbndk supports it and it requires less work on our part since the bbdnk will validate it anyways. You can set the hostname using a hosts file or DNS. I'm not sure about changing the hostname on the device, but it's possible that there's one pre-defined that we don't know about.
There was a problem hiding this comment.
As discussed, ip validation removed to allow support hostnames as well, i.e. just pass the param to blackberry-deploy
|
Reviewed. |
|
@dlifshitz-maca: Why we need to use --device_password? As I mentioned simulator also uses password protection. |
|
Merged with @dlifshitz-maca updated stuff |
|
We have to use --device_password because that's what we told Appcelerator to use. This is written in the Jira comments. Also, the bbndk uses "device" to mean both a physical device and the simulator. |
|
@dlifshitz-maca Updated to --device_password |
|
Updated |
| if (args.subparser_name == 'run'): | ||
| builder = Builder(args.project_path.decode('utf-8'), args.type.decode('utf-8'), bbndk, args.ip_address.decode('utf-8'), args.password.decode('utf-8') if args.password != None else None) | ||
| else: | ||
| builder = Builder(args.project_path.decode('utf-8'), args.type.decode('utf-8'), bbndk) |
There was a problem hiding this comment.
This if else could be merged with the following one as there are testing the same condition. Better yet would be to supply the extra ip_address, and password args directly to builder.run() instead of to the ctor.
Doesn't need to be done in this patch or could be done at merge time.
|
Approved with comment Also don't forget to wait for david's patches to get merged first before merging this PR as it has dependancies on David's patches |
|
Updated |
|
Reapproved, let's just make sure the dependencies are merged first ;) |
|
Approved, though you need to get rid of the circular dependency. |
…ce/simulator) and (optionally) password ver4 Reviewers: JP, David L ChangeLog - Remove ip validation to support hostnames - Update --password to --device_password Tests 1) Create project using project.py script 2) Run builder.py by providing ip address and password (optionally, if set in simulator/device) 3) Make sure application successfully built and installed on simulator/device 4) Make sure launch failed if password set, but doesn't provided 5) Make sure launch succeeded if password not set, but provided 6) Make sure ip validation returns error on non-valid ip address Conflicts: support/blackberry/builder.py
…ce/simulator) and (optionally) password ver5 Reviewers: JP, David L ChangeLog - Minor improvements [Tests] 1) Create project using project.py script 2) Run builder.py by providing ip address and password (optionally, if set in simulator/device) 3) Make sure application successfully built and installed on simulator/device 4) Make sure launch failed if password set, but doesn't provided 5) Make sure launch succeeded if password not set, but provided 6) Make sure ip validation returns error on non-valid ip address Conflicts: support/blackberry/builder.py
…rry' into reviewBranch #Some more info....
TIMOB-8921: Blackberry: Add functionality to specify ip address (device/simulator) and (optionally) password
Reviewers: JP, David L
NOTE: Review only 95d8a2a1a3c9dfabcea7e75e2de39e2b65b4293b commit
ChangeLog
blackberryndk.py
Added password parameter to deploy function
Added password parameter to deploy command if provided
builder.py
Added ip address & password parameters to script
Remove hardcoded ip address
[Tests]