TheRacetrack / plugin-python-job-type

A plugin for Racetrack which extends it with Python Job Type
Apache License 2.0
0 stars 1 forks source link

url /pub/job backwards compatibiltiy #13

Closed jsandroos closed 1 year ago

jsandroos commented 1 year ago

Hi @LookACastle

Regarding the fatman -> job degreasing, we had discussed backwards compatibility. This has been interpreted in the widest possible terms in the Lab and hence people have scripts calling the api endpoints, e.g. /pub/fatman///health, like so:

curl -X 'GET' \ '<dev url>/pub/job/<name>/<version>/health' \ -H 'accept: application/json'

which now fail with: {"detail":"Not Found"}

I suspect this is due to the api endpoint no longer being hosted at /pub/fatman but only at /pub/job.

Should we add backwards compatibility here?

LookACastle commented 1 year ago

It definitely should be backwards compatible, and I was under the impression that I had made sure it was. Thanks for the bug report, I'm on the case.

iszulcdeepsense commented 1 year ago

I'm confident that we kept the old endpoint for backward compatibility in PUB.

iszulcdeepsense commented 1 year ago

Ah, maybe python3 / mllab plugin is worth to check, cause maybe it's not an error raised by PUB, but the Job itself.

LookACastle commented 1 year ago

I just tested it locally with adder (via make up test) and it does not seem to work. pub/router.go lines 26-32 should add the endpoints though, so I'm not quite sure what's happening there.

iszulcdeepsense commented 1 year ago

Right, PUB is fine and it's definitely the plugins' fault:

https://github.com/TheRacetrack/plugin-python-job-type/blob/master/python3-job-type/python_wrapper/job_wrapper/api.py#L69 (there is only the newer path)

iszulcdeepsense commented 1 year ago

Fortunately, mount_at_base_path can accept multiple base path patterns as an argument: https://github.com/TheRacetrack/plugin-python-job-type/blob/04bf59634ee23645d46eb16099304c7593905f67/python3-job-type/python_wrapper/racetrack_commons/api/asgi/proxy.py#L6

So changing this line https://github.com/TheRacetrack/plugin-python-job-type/blob/04bf59634ee23645d46eb16099304c7593905f67/python3-job-type/python_wrapper/job_wrapper/api.py#L97 should do the trick.

JosefAssadERST commented 1 year ago

Would a test that would have caught this detail?

LookACastle commented 1 year ago

It's possible. There's a decent chance such a test would've had the endpoint it was contacting renamed as well though.

iszulcdeepsense commented 1 year ago

Turns out the unit test wasn't checking against any base path. Now it's covered by https://github.com/TheRacetrack/plugin-python-job-type/pull/14/commits/924638233aab47ab0f6c2fce10f66573a5825336