Workloads update #13
Workloads update #13
Conversation
mdgreenwald
force-pushed
the
matthew/workloads_update
branch
from
40c134a to
4ad8ce1
Compare
Signed-off-by: mdgreenwald <[email protected]>
mdgreenwald
force-pushed
the
matthew/workloads_update
branch
from
4ad8ce1 to
e05eea7
Compare
| g.PD_REGISTRY = current_app.config['PD_REGISTRY'] | ||
|
|
||
| def read_deployment(deployment, namespace): | ||
| def list_cron_job(name, namespace): |
There was a problem hiding this comment.
All of these list_* methods seem to follow the same pattern.
I could see a more general fetch_object(object, name, namespace) (something like that) that would dispatch the calls to the necessary k8s api endpoint. At that point, you could either use the more generic fetch_object or keep the more specialized calls that :just: use the generic one under the hood.
| ) | ||
| if len(api_response.items) == 1: | ||
| return api_response.items[0] | ||
| else: |
There was a problem hiding this comment.
I'm not sure if returning a string for the else clause is the best approach. Your consumer of that calls a long chain of methods, e.g. cron_job_object.spec.job_template.spec.template.spec.containers[0].image which will probably blow up if cron_job_object were a string.
Do you want it to raise an exception as opposed to returning a string? I'm not sure if that's considered best practice in Python, but that might be an option.
| image_tag = request.args['image_tag'] | ||
| image_name = request.args['image_name'] | ||
| name = request.args['name'] | ||
| namespace = request.args['namespace'] |
There was a problem hiding this comment.
Unless this does something special like ensure that the parameters are passed, these assignments feel noisy any unnecessary since you "consume them" just a few lines below.
There was a problem hiding this comment.
That style actually does force the request to pass those parameters.
| name = request.args['name'] | ||
| namespace = request.args['namespace'] | ||
| cronjob = patch_cron_job( | ||
| cron_job_object=list_cron_job(name=name, namespace=namespace), |
There was a problem hiding this comment.
patch_cron_job should probably be responsible for fetching the cron_job_object. It's signature already takes a name & namespace so you wouldn't have to add anything, but you would be able to remove something and simplify the call sites.
| @@ -0,0 +1,89 @@ | |||
| --- | |||
There was a problem hiding this comment.
Can I run these tests? If so, maybe a few details in the README would go a long way.
|
@theIV I am going to leave these suggestions for a later PR which will add testing and linting. Thank you for your helpful feedback! |
Changes
/api/v1/deployin favor of/api/v1/deployment.1.1.2.tests/setup.yamlto create test workloads to patch.object_typevariable tonamefor clarity.Deprecation notice
The original deploy endpoint
/api/v1/deploywill continue to work for the foreseeable future, but it is recommended to move to the new deployment/api/v1/deploymentendpoint.