Add pipelines 'get' and 'reset' commands by mukulmurthy · Pull Request #273 · databricks/databricks-cli

mukulmurthy · 2019-12-06T09:38:57Z

Add the commands databricks pipelines get and databricks pipelines reset to get and reset Delta Pipelines
Stop double-passing credentials to the Delta Pipelines service
Improve CLI documentation.

Tested with new and additional unit tests.

mukulmurthy · 2019-12-06T18:57:19Z

@anew and @arulajmani as potential reviewers too

anew · 2019-12-09T17:48:13Z

+@click.command(context_settings=CONTEXT_SETTINGS,
+               short_help='Gets a delta pipeline\'s current spec and status')
+@click.argument('spec_arg', default=None, required=False)
+@click.option('--spec', default=None, type=PipelineSpecClickType(), help=PipelineSpecClickType.help)


Does it make sense to provide the spec to an API that retrieves the spec? Perhaps we should only accept a pipeline id as argument here.

It also gets the status. Arul and Michael and I debated a bunch when we first did this for delete, but we figured it was probably best to support all options there. I think we should do the same here, but check in with early customers and see if this is making it easier or harder to use.

Classic REST will assign a unique ID on resource creation and then use that ID for subsequent requests.

Accepting the entire spec for a delete() or similar call may seem harmless. But I have always believed that a good API has exactly one way to do a specific thing. Allowing to do it in multiple ways creates ambiguity, it can cause confusion, and it multiplies the amount of code that needs to be maintained (and kept backwards-compatible) in the future.

One drawback of accepting a spec for get or delete etc. is that the user may think the entire spec must match, when in reality everything but ID is ignored. For example, a user may expect that the spec returned is equal to the spec passed in - that would not be the case. Or, on delete, the user may expect that it only deletes resources that match the whole spec, including, say, the name. But it will delete a pipeline with a matching ID, regardless of name.

This kind of ambiguities can easily be avoided.

anew · 2019-12-09T17:49:28Z

+@click.command(context_settings=CONTEXT_SETTINGS,
+               short_help='Resets a delta pipeline so data can be reprocessed from scratch')
+@click.argument('spec_arg', default=None, required=False)
+@click.option('--spec', default=None, type=PipelineSpecClickType(), help=PipelineSpecClickType.help)


same here - I think it the pipeline id is the only argument we should accept here.
And the same applies to delete...

anew · 2019-12-09T17:54:33Z

-    def delete(self, pipeline_id=None, credentials=None, headers=None):
+    def delete(self, pipeline_id=None, headers=None):
        _data = {}
        if pipeline_id is not None:


I'd expect that pipeline_id must not be None.

anew · 2019-12-09T17:54:59Z

+    def reset(self, pipeline_id=None, headers=None):
+        _data = {}
+        if pipeline_id is not None:
+            _data['pipeline_id'] = pipeline_id


anew · 2019-12-09T17:59:15Z

+
+@provide_conf
+def test_get_cli_spec_option(pipelines_api_mock, tmpdir):
+    path = tmpdir.join('/spec.json').strpath


some of these tests become unnecessary when you allow only the pipeline id as argument

anew · 2019-12-09T17:59:47Z

            path = '{}/{}.{}'.format(base_pipelines_dir, file_hash, extension)
        return path

-    @staticmethod


this change seems unrelated - how come we don't need this any more?

This is for the

Stop double-passing credentials to the Delta Pipelines service
part of the change. This method pulled CLI credentials from the config file so they could be included in the body of the request. Now that we don't need to include them in the body, we don't need this method. Arul wrote it nicely so that we could just whack this method once we fixed the credentials server-side situation.

ok. I think it is better to make these kind of refactorings in a separate PR, but no biggie.

mukulmurthy

Responded to Andreas's comments

mukulmurthy · 2019-12-10T01:12:38Z

            path = '{}/{}.{}'.format(base_pipelines_dir, file_hash, extension)
        return path

-    @staticmethod


This is for the

Stop double-passing credentials to the Delta Pipelines service
part of the change. This method pulled CLI credentials from the config file so they could be included in the body of the request. Now that we don't need to include them in the body, we don't need this method. Arul wrote it nicely so that we could just whack this method once we fixed the credentials server-side situation.

mukulmurthy · 2019-12-10T01:15:29Z

+@click.command(context_settings=CONTEXT_SETTINGS,
+               short_help='Gets a delta pipeline\'s current spec and status')
+@click.argument('spec_arg', default=None, required=False)
+@click.option('--spec', default=None, type=PipelineSpecClickType(), help=PipelineSpecClickType.help)


It also gets the status. Arul and Michael and I debated a bunch when we first did this for delete, but we figured it was probably best to support all options there. I think we should do the same here, but check in with early customers and see if this is making it easier or harder to use.

anew

Let's defer the API discussion (whether to take a spec or a pipeline id as arguments for get/delete/reset) and get this merged.

LGTM

* Add the commands `databricks pipelines get` and `databricks pipelines reset` to get and reset Delta Pipelines * Stop double-passing credentials to the Delta Pipelines service * Improve CLI documentation. Tested with new and additional unit tests.

Mukul Murthy added 4 commits December 4, 2019 17:47

new service.py file for pipelines

0c536da

get rid of credentials from pipelines

6396773

get and reset APIs

bba4a11

adding better help for CLI commands

9874f20

mukulmurthy requested a review from andrewmchen December 6, 2019 18:20

anew reviewed Dec 9, 2019

View reviewed changes

mukulmurthy commented Dec 10, 2019

View reviewed changes

anew approved these changes Dec 11, 2019

View reviewed changes

mukulmurthy merged commit 2c8d637 into databricks:pipelines-client Dec 11, 2019

mukulmurthy deleted the nocred branch December 11, 2019 22:20

Conversation

mukulmurthy commented Dec 6, 2019

Uh oh!

mukulmurthy commented Dec 6, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anew Dec 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mukulmurthy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

anew left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

anew Dec 10, 2019 •

edited

Loading