Bug 208746 – resultsdpy: Add script to run local instance

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Jonathan Bedard]

There should be a script to run a local instance of the results database. I've had various versions of this script floating around, this is an attempt to clean it up and commit it.

[Jonathan Bedard]

Created attachment 392808 [details]
Patch

[Aakash Jain]

Comment on attachment 392808 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=392808&action=review

rs=me

> Tools/resultsdbpy/resultsdbpy/run:77
> +    parser.set_defaults(redis_server='localhost' if Docker.installed() else 'mock')

if docker is installed and user passed --mock-redis, will this cause the --mock-redis to be ignored?

> Tools/resultsdbpy/resultsdbpy/run:84
> +        '--no-drop-keyspace', help='Do NOT drop the existing keyspace before running the program',

can/should we make --drop-keyspace and --no-drop-keyspace mutually exclusive?

> Tools/resultsdbpy/resultsdbpy/run:115
> +    from example.worker import main

should this script run example worker by default?

> Tools/resultsdbpy/resultsdbpy/example/environment.py:54
> +        self.redis_port = os.environ.get('REDIS_PORT', 6379)

might be a good idea to make default port 6379 a class variable.

> Tools/resultsdbpy/resultsdbpy/example/environment.py:101
> +            result += f"    {key.upper()}: {self.obfuscate_password(value) if ('password' in key or 'token' in key or key.endswith('key')) and value else value}\n"

better to split it in multiple lines for readability.

> Tools/resultsdbpy/resultsdbpy/example/environment.py:142
> +            default_ttl_seconds=Model.TTL_YEAR * 5,

this duration should be a class variable, not hidden inside here.

> Tools/resultsdbpy/resultsdbpy/example/environment.py:149
> +    print('Environment for setup:')

is this print supposed to be committed or was for debugging?

> Tools/resultsdbpy/resultsdbpy/example/main.py:28
> +from flask_cors import CORS

seems unused. Can you audit all the imports.

> Tools/resultsdbpy/resultsdbpy/example/main.py:31
> +from werkzeug.exceptions import HTTPException

Do we need to install something for this? This doesn't seems to be used.

> Tools/resultsdbpy/resultsdbpy/example/main.py:36
> +print(environment)

Is this print supposed to be committed? Also, it can be one-liner.

> Tools/resultsdbpy/resultsdbpy/example/worker.py:32
> +    print(environment)

These two print statements can be combines into one. Also is this print supposed to be committed or was only for debugging?

[Jonathan Bedard]

Comment on attachment 392808 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=392808&action=review

>> Tools/resultsdbpy/resultsdbpy/run:77
>> +    parser.set_defaults(redis_server='localhost' if Docker.installed() else 'mock')
> 
> if docker is installed and user passed --mock-redis, will this cause the --mock-redis to be ignored?

No, that's why we're just setting the default.

Basically, --local-redis won't work if docker isn't installed.

>> Tools/resultsdbpy/resultsdbpy/run:84
>> +        '--no-drop-keyspace', help='Do NOT drop the existing keyspace before running the program',
> 
> can/should we make --drop-keyspace and --no-drop-keyspace mutually exclusive?

Can, yes.

Should? Not sure about more than we already are. Pretty much every one of these options has the same problem. What argparse will do, if I'm not mistaken, is respect the last flag passed. Which seems acceptable.

>> Tools/resultsdbpy/resultsdbpy/run:115
>> +    from example.worker import main
> 
> should this script run example worker by default?

Yes, the service won't work otherwise.

>> Tools/resultsdbpy/resultsdbpy/example/environment.py:149
>> +    print('Environment for setup:')
> 
> is this print supposed to be committed or was for debugging?

It is deliberate.

This is essentially the master record of what's actually going to be run. Importantly, actual infrastructure does this.

>> Tools/resultsdbpy/resultsdbpy/example/main.py:31
>> +from werkzeug.exceptions import HTTPException
> 
> Do we need to install something for this? This doesn't seems to be used.

I will audit these. Copied from the production instances. which have extra authentication.

>> Tools/resultsdbpy/resultsdbpy/example/main.py:36
>> +print(environment)
> 
> Is this print supposed to be committed? Also, it can be one-liner.

It is deliberate.

This is essentially the master record of what's actually going to be run. Importantly, actual infrastructure does this.

>> Tools/resultsdbpy/resultsdbpy/example/worker.py:32
>> +    print(environment)
> 
> These two print statements can be combines into one. Also is this print supposed to be committed or was only for debugging?

It is deliberate.

This is essentially the master record of what's actually going to be run. Importantly, actual infrastructure does this.

[Jonathan Bedard]

Created attachment 393407 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 393407 [details]
Patch for landing

Clearing flags on attachment: 393407

Committed r258355: <https://trac.webkit.org/changeset/258355>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/60386306>