Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Accept JSON policy object in cluster policy commands #557

Merged
merged 1 commit into from
Oct 6, 2022
Merged

Accept JSON policy object in cluster policy commands #557

merged 1 commit into from
Oct 6, 2022

Conversation

copdips
Copy link
Contributor

@copdips copdips commented Sep 2, 2022 •
edited
Loading

@copdips copdips force-pushed the feat/#554/accept-dict-cluster-policy-definition branch 2 times, most recently from 41ee6d8 to 81f0783 Compare September 2, 2022 21:50
@copdips
Copy link
Contributor Author

copdips commented Sep 19, 2022

@pietern
could you please review ?

pietern
pietern requested changes Sep 27, 2022
View reviewed changes
Copy link
Contributor

@pietern pietern left a comment

Choose a reason for hiding this comment

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

Thanks for making the PR, @copdips. Apologies for the longer turn around time.

databricks_cli/cluster_policies/api.py Outdated Show resolved Hide resolved
databricks_cli/cluster_policies/api.py Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Sep 27, 2022 •
edited
Loading

Codecov Report

Base: 75.39% // Head: 75.49% // Increases project coverage by +0.09% 🎉

Coverage data is based on head (300475c) compared to base (81c26a8).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #557      +/-   ##
==========================================
+ Coverage   75.39%   75.49%   +0.09%     
==========================================
  Files          55       55              
  Lines        4817     4823       +6     
==========================================
+ Hits         3632     3641       +9     
+ Misses       1185     1182       -3
Impacted Files Coverage Δ
databricks_cli/cluster_policies/api.py 85.00% <100.00%> (+27.85%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@copdips copdips force-pushed the feat/#554/accept-dict-cluster-policy-definition branch 4 times, most recently from 87637a7 to c43d597 Compare September 27, 2022 13:47
@copdips copdips force-pushed the feat/#554/accept-dict-cluster-policy-definition branch 2 times, most recently from 92af952 to 4152d60 Compare October 6, 2022 09:50
@copdips
Copy link
Contributor Author

copdips commented Oct 6, 2022

@pietern
please recheck now

@pietern
Copy link
Contributor

pietern commented Oct 6, 2022

@copdips Thanks for updating. Everything looks good to me. There's one failing test that alerts on function prototype changes compare to master and it alerts on the argument name being changes from json to policy. Could you revert that rename? That way, if anyone depends on these APIs and uses keyword args, their calls won't break.

@copdips copdips force-pushed the feat/#554/accept-dict-cluster-policy-definition branch 2 times, most recently from 8b6c913 to e07145d Compare October 6, 2022 12:54
@copdips copdips force-pushed the feat/#554/accept-dict-cluster-policy-definition branch from e07145d to 300475c Compare October 6, 2022 12:56
@copdips
Copy link
Contributor Author

copdips commented Oct 6, 2022

@copdips Thanks for updating. Everything looks good to me. There's one failing test that alerts on function prototype changes compare to master and it alerts on the argument name being changes from json to policy. Could you revert that rename? That way, if anyone depends on these APIs and uses keyword args, their calls won't break.

Thx the review, I reverted the rename, and I also changed import json to from json import dumps as json_dumps, otherwise pylint won't be happy

@pietern pietern changed the title feat/#554/accept-dict-cluster-policy-definition Accept JSON policy object in cluster policy commands Oct 6, 2022
pietern
pietern approved these changes Oct 6, 2022
View reviewed changes
Copy link
Contributor

@pietern pietern left a comment

Choose a reason for hiding this comment

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

Thank you, @copdips ! Looks great.

@pietern pietern merged commit c62d861 into databricks:main Oct 6, 2022
@copdips copdips mentioned this pull request Oct 14, 2022
Closed
andyl-db pushed a commit to andyl-db/databricks-cli that referenced this pull request Oct 20, 2022
@copdips @andyl-db
Accept JSON policy object in cluster policy commands (databricks#557)
b94021b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pietern pietern pietern approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@copdips @codecov-commenter @pietern