Skip to content

Interpret pipeline libraries relative to spec#368

Merged
mukulmurthy merged 3 commits intodatabricks:masterfrom
anew:spec-relative
Apr 27, 2021
Merged

Interpret pipeline libraries relative to spec#368
mukulmurthy merged 3 commits intodatabricks:masterfrom
anew:spec-relative

Conversation

@anew
Copy link
Copy Markdown
Contributor

@anew anew commented Apr 27, 2021

The library paths in a pipeline spec should be interpreted as relative to the spec's location, and not relative to the current working directory.

@anew
Copy link
Copy Markdown
Contributor Author

anew commented Apr 27, 2021

@mukulmurthy please take a look

runner = CliRunner()
runner.invoke(cli.deploy_cli, [path])
assert pipelines_api_mock.create.call_args_list[0][0][1] is False
assert pipelines_api_mock.create.call_args_list[0][0][2] is False
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are these changes?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These test that the allow_duplicate_names parameter of create and deploy is set correctly. Because I added a new parameter spec_dir, this shifts by one from position 1 to 2.

spec['libraries'] = []

pipelines_api.create(spec, allow_duplicate_names=False)
pipelines_api.create(spec, spec_dir='.', allow_duplicate_names=False)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If all these tests assume that the spec dir is the same as the current dir, would any of them actually fail even without the code changes?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these tests use a spec without libraries, so it makes no difference to them.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So which one's the test that would fail without the prod code changes?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

expected_data['allow_duplicate_names'] = allow_duplicate_names

api_method(spec, allow_duplicate_names)
api_method(spec, tmpdir.strpath, allow_duplicate_names)
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's not obvious, but api_method is either create or deploy. So this is the test that would actually fail.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, didn't realize that you switched the spec to use a relative path to itself rather than to the cwd.

@mukulmurthy mukulmurthy merged commit f047ba8 into databricks:master Apr 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants