-
Notifications
You must be signed in to change notification settings - Fork 3
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.
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
[JOBS-13997] Add SdkJobsUtils and fix taskValuesUtils in proxy mode #33
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should limit our changes to introducing an implementation of TaskValuesUtils
and not make changes to CommandContext
.
databricks-dbutils-scala/src/main/scala/com/databricks/sdk/scala/dbutils/DBUtils.scala
Outdated
Show resolved
Hide resolved
private var commandContext = | ||
CommandContext() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need an instance here? It seems our integration does not rely on it. Can this be private var commandContext: CommandContext = _
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I check that initially getContext
returns empty context
databricks-dbutils-scala/src/main/scala/com/databricks/sdk/scala/dbutils/SdkDBUtilsImpl.scala
Outdated
Show resolved
Hide resolved
databricks-dbutils-scala/src/main/scala/com/databricks/sdk/scala/dbutils/SdkDBUtilsImpl.scala
Outdated
Show resolved
Hide resolved
...c/test/scala/com/databricks/sdk/scala/dbutils/integration/SdkTaskValuesIntegrationTest.scala
Outdated
Show resolved
Hide resolved
...c/test/scala/com/databricks/sdk/scala/dbutils/integration/SdkTaskValuesIntegrationTest.scala
Outdated
Show resolved
Hide resolved
...c/test/scala/com/databricks/sdk/scala/dbutils/integration/SdkTaskValuesIntegrationTest.scala
Outdated
Show resolved
Hide resolved
...c/test/scala/com/databricks/sdk/scala/dbutils/integration/SdkTaskValuesIntegrationTest.scala
Outdated
Show resolved
Hide resolved
...c/test/scala/com/databricks/sdk/scala/dbutils/integration/SdkTaskValuesIntegrationTest.scala
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setJson
should not throw, otherwise the SDK cannot be used for local development when the customer relies on setJson
on top of DBR.
I will defer final review to @mgyucht.
databricks-dbutils-scala/src/main/scala/com/databricks/sdk/scala/dbutils/SdkDBUtilsImpl.scala
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple comments, thanks for the contribution!
Changes
taskValues
in proxy mode, because it is not field, but methodTests