CLI support for setting Delta Sharing recipient properties

[zhuansunxt]

Add --property param to both recipient create and update command following the pattern of allowed_ip_address

[zhuansunxt]

@wchau do you know who else should I send for review or is it enough to get reviewed within our team?

[wchau]

@wchau do you know who else should I send for review or is it enough to get reviewed within our team?

Added @pietern

[andyl-db]

Maybe --property key value or --property key=value would be easier for users than specifying a JSON each time.

https://stackoverflow.com/questions/51164033/python-click-multiple-key-value-pair-arguments

Though the benefit of the JSON is supporting multiple future properties, but are we planning on other properties to the JSON?

[zhuansunxt]

Good idea. I changed it to be separated by the equal sign key=value with a caveat that it won't support setting = as part of the key or value which I think is probably fine.

[andyl-db]

LGTM

[pietern]

Couple remarks:

• Please name this properties to match what it's called at the API level. Why custom?
• Please add a test for passing x=y=z to this argument to make it works.
• Nit: please trim trailing whitespace. It adds diffs to future commits.

[codecov-commenter]

Codecov Report

Base: 75.76% // Head: 75.80% // Increases project coverage by +0.04% 🎉

Coverage data is based on head (b69d429) compared to base (7019590).
Patch coverage: 84.21% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #584      +/-   ##
==========================================
+ Coverage   75.76%   75.80%   +0.04%     
==========================================
  Files          55       55              
  Lines        4901     4922      +21     
==========================================
+ Hits         3713     3731      +18     
- Misses       1188     1191       +3     

Impacted Files	Coverage Δ
databricks_cli/unity_catalog/uc_service.py	27.30% <0.00%> (-0.22%)	⬇️
databricks_cli/unity_catalog/api.py	47.97% <50.00%> (ø)
databricks_cli/unity_catalog/delta_sharing_cli.py	85.75% <100.00%> (+0.53%)	⬆️
databricks_cli/jobs/cli.py	96.07% <0.00%> (-0.55%)	⬇️

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.

[zhuansunxt]

@pietern See some questions below.

Couple remarks:

• Please name this properties to match what it's called at the API level. Why custom?

Initially I named it property but that was a keyword redefine. And I thought the name should NOT be of plural form since user are specifying one property at a time?

• Please add a test for passing x=y=z to this argument to make it works.

Added a test.

• Nit: please trim trailing whitespace. It adds diffs to future commits.

Dumb question: how do you identify all lines with trailing whitespaces? I can tell by either look at the github diff or the local code.

[pietern]

Thanks!

Yeah singular is better. Can you elaborate on the keyword redefine issue?

[zhuansunxt]

@pietern If I use the name property the linter will report an error complaining about Redefining built-in 'property'. It looks like some reserved keyword in python.

[zhuansunxt]

@wchau @pietern can you take another look?

[pietern]

I understand the issue with the keyword, but that doesn't mean we have to use "custom" in the name of the argument and the help texts. We should stick to the naming we use in the documentation. The variable can be named whatever works that isn't property.

Otherwise a user may have questions such as:

• why can't I set properties, and only custom properties?
• what is the difference between properties and custom properties?

[zhuansunxt]

The variable can be named whatever works that isn't property

@pietern With the constraints of 1) can not override property 2) must be singular, custom-property is the best name I can think of as the product owner. A recipient can have pre-defined recipients and custom properties, and only the latter can be set by the end users (pre-defined properties are system-generated and can't be mutated). It WILL align with the public documentation we publish at a later point.

[pietern]

Argument help was lagging the argument name. Replaced in suggestions.