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.

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

Add run_as and run_as_user_name field to job definition #2366

Closed
wants to merge 7 commits into from

Conversation

dvkuznietsov
Copy link
Contributor

@dvkuznietsov dvkuznietsov commented Jun 2, 2023

Changes

Added read-only runAsUserName field to job definition and write-only runAs field to job settings definition. Fields are added in openAPI spec and are marked as private preview.

Tests

Tested e2e by running terraform apply on a dev workspace

  • make test run locally
  • relevant change in docs/ folder
  • covered with integration tests in internal/acceptance
  • relevant acceptance tests are passing
  • using Go SDK

@codecov-commenter
Copy link

Codecov Report

Merging #2366 (d24f567) into master (48e7be8) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2366   +/-   ##
=======================================
  Coverage   88.53%   88.53%           
=======================================
  Files         141      141           
  Lines       11620    11620           
=======================================
  Hits        10288    10288           
  Misses        886      886           
  Partials      446      446           
Impacted Files Coverage Δ
jobs/resource_job.go 95.79% <ø> (ø)

Copy link
Contributor

@gaborratky-db gaborratky-db left a comment

Choose a reason for hiding this comment

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

LGTM with nits

docs/resources/job.md Outdated Show resolved Hide resolved
docs/resources/job.md Outdated Show resolved Hide resolved
Co-authored-by: Gabor Ratky <gabor.ratky@databricks.com>
Copy link
Contributor

@alexott alexott left a comment

Choose a reason for hiding this comment

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

in general looks good, just few comments

@@ -88,6 +88,7 @@ The resource supports the following arguments:
* `email_notifications` - (Optional) (List) An optional set of email addresses notified when runs of this job begins, completes and fails. The default behavior is to not send any emails. This field is a block and is documented below.
* `webhook_notifications` - (Optional) (List) An optional set of system destinations (for example, webhook destinations or Slack) to be notified when runs of this job begins, completes and fails. The default behavior is to not send any notifications. This field is a block and is documented below.
* `schedule` - (Optional) (List) An optional periodic schedule for this job. The default behavior is that the job runs when triggered by clicking Run Now in the Jobs UI or sending an API request to runNow. This field is a block and is documented below.
* `run_as` - (Optional) An optional identifier of the user or the service principal that the job runs as. If not specified, the job runs as the user who created the job. This field is a block and is documented below.
Copy link
Contributor

Choose a reason for hiding this comment

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

Usually this list includes only direct attributes, and blocks are documented as separate sections.

Comment on lines +204 to +205
UserName string `json:"user_name,omitempty"`
ServicePrincipalName string `json:"service_principal_name,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

we may need to add a customization to set ConflictsWith between these two fields. Also, we may need to add checks that both strings aren't empty...

@alexott
Copy link
Contributor

alexott commented Jun 6, 2023

Please rebase on the latest master...

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.

5 participants