Skip to content

Add/Fix support for maven deps in pipelines spec#299

Merged
mukulmurthy merged 4 commits intodatabricks:pipelines-clientfrom
null-sleep:dhruv/add-support-for-maven-deps
May 29, 2020
Merged

Add/Fix support for maven deps in pipelines spec#299
mukulmurthy merged 4 commits intodatabricks:pipelines-clientfrom
null-sleep:dhruv/add-support-for-maven-deps

Conversation

@null-sleep
Copy link
Copy Markdown
Contributor

@null-sleep null-sleep commented May 27, 2020

Previously a pipelines spec with maven dependencies would fail with the error: Error: AttributeError: 'dict' object has no attribute 'decode'. This PR fixes the bugs.

Testing:
Updated tests and manual testing

BUFFER_SIZE = 1024 * 64
base_pipelines_dir = 'dbfs:/pipelines/code'
supported_lib_types = {'jar', 'whl'}
supported_lib_types = {'jar', 'whl', 'maven'}
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.

add egg here as well?

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.

When we did python before we had a good reason for not supporting eggs. I don't remember exactly what that was, but we can do it in the future when it's needed.

@null-sleep null-sleep force-pushed the dhruv/add-support-for-maven-deps branch from 314017c to a5ae253 Compare May 27, 2020 10:19
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 27, 2020

Codecov Report

Merging #299 into pipelines-client will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@                 Coverage Diff                  @@
##           pipelines-client     #299      +/-   ##
====================================================
+ Coverage             84.60%   84.62%   +0.01%     
====================================================
  Files                    35       35              
  Lines                  2391     2394       +3     
====================================================
+ Hits                   2023     2026       +3     
  Misses                  368      368              
Impacted Files Coverage Δ
databricks_cli/pipelines/api.py 97.53% <100.00%> (+0.09%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 45e62c3...2a09549. Read the comment docs.

@null-sleep
Copy link
Copy Markdown
Contributor Author

@mukulmurthy

Copy link
Copy Markdown
Collaborator

@mukulmurthy mukulmurthy left a comment

Choose a reason for hiding this comment

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

Minor: The coordinates field is supposed to include the version (https://docs.databricks.com/dev-tools/api/latest/libraries.html#mavenlibrary). Can you add some simple version here? We can merge this after

Comment thread tests/pipelines/test_api.py Outdated
Copy link
Copy Markdown
Collaborator

@mukulmurthy mukulmurthy left a comment

Choose a reason for hiding this comment

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

LGTM. I merged your other PR, so can you merge pipelines-client into this PR to make sure everything builds and passes tests together? Can merge this after that.

@null-sleep null-sleep requested a review from mukulmurthy May 29, 2020 19:40
@mukulmurthy mukulmurthy merged commit e0c7ce0 into databricks:pipelines-client May 29, 2020
@null-sleep null-sleep deleted the dhruv/add-support-for-maven-deps branch May 30, 2020 04:16
mukulmurthy pushed a commit that referenced this pull request Jun 1, 2020
Previously a pipelines spec with maven dependencies would fail with the error: Error: AttributeError: 'dict' object has no attribute 'decode'. This PR fixes the bugs.

Testing:
Updated tests and manual testing
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.

3 participants