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

Allow overriding compute for non-development mode targets #1899

Conversation

lennartkats-db
Copy link
Contributor

@lennartkats-db lennartkats-db commented Nov 13, 2024

Changes

Allow overriding compute for non-development targets. We previously had a restriction in place where --cluster-id was only allowed for targets that use mode: development. The intention was to prevent mistakes, but this was overly restrictive.

Tests

Updated unit tests.

bundle/config/mutator/override_compute.go Outdated Show resolved Hide resolved
bundle/config/mutator/override_compute.go Outdated Show resolved Hide resolved
}
if v := env.Get(ctx, "DATABRICKS_CLUSTER_ID"); v != "" {
b.Config.Bundle.ClusterId = v
}

if b.Config.Bundle.ClusterId == "" {
return nil
return diags
}

r := b.Config.Resources
for i := range r.Jobs {
overrideJobCompute(r.Jobs[i], b.Config.Bundle.ClusterId)
Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated note: this also overrides serverless notebook tasks (without environment_key).

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.

Functionality LGTM, approving to unblock.

Please take a look at:

  • The flag name in the PR summary doesn't exist (it's --cluster-id)
  • Wording in the new log talks about "compute"

bundle/config/mutator/override_compute.go Outdated Show resolved Hide resolved
Copy link

github-actions bot commented Dec 4, 2024

If integration tests don't run automatically, an authorized user can run them manually by following the instructions below:

Trigger:
go/deco-tests-run/cli

Inputs:

  • PR number: 1899
  • Commit SHA: b624ce999a9ae3ddaad2b0bc85218810c14f4371

Checks will be approved automatically on success.

@eng-dev-ecosystem-bot
Copy link
Collaborator

Test Details: go/deco-tests/12243557365

@pietern
Copy link
Contributor

pietern commented Dec 9, 2024

@lennartkats-db I triggered integration tests (see the comment, it doesn't auto-trigger from forks).

But then I tried it out and found the issue above, so I disabled auto-merge.

@pietern pietern disabled auto-merge December 9, 2024 20:26
@pietern
Copy link
Contributor

pietern commented Dec 9, 2024

@denik FYI, no review needed.

@lennartkats-db lennartkats-db added this pull request to the merge queue Dec 10, 2024
Merged via the queue into databricks:main with commit f3c628e Dec 10, 2024
11 checks passed
@lennartkats-db lennartkats-db deleted the relax-compute-override-requirements branch December 10, 2024 10:09
github-merge-queue bot pushed a commit that referenced this pull request Dec 11, 2024
…1994)

## Changes

We should show a warning when using a cluster override with 'mode:
production'. Right now, we inadvertently show an error for this state.
This is a followup based on
#1899 (comment).
andrewnester added a commit that referenced this pull request Dec 18, 2024
Bundles:
 * Allow overriding compute for non-development mode targets ([#1899](#1899)).
 * Avoid panic if Config.Workspace.CurrentUser.User is not set ([#1993](#1993)).
 * Show an error when using a cluster override with 'mode: production' ([#1994](#1994)).

Internal:

API Changes:
 * Added `databricks account federation-policy` command group.
 * Added `databricks account service-principal-federation-policy` command group.
 * Added `databricks aibi-dashboard-embedding-access-policy delete` command.
 * Added `databricks aibi-dashboard-embedding-approved-domains delete` command.

OpenAPI commit a6a317df8327c9b1e5cb59a03a42ffa2aabeef6d (2024-12-16)
Dependency updates:
 * Upgrade TF provider to 1.62.0 ([#2030](#2030)).
 * Upgrade Go SDK to 0.54.0 ([#2029](#2029)).
 * Bump TF codegen dependencies to latest ([#1961](#1961)).
 * Bump golang.org/x/term from 0.26.0 to 0.27.0 ([#1983](#1983)).
 * Bump golang.org/x/sync from 0.9.0 to 0.10.0 ([#1984](#1984)).
 * Bump github.com/databricks/databricks-sdk-go from 0.52.0 to 0.53.0 ([#1985](#1985)).
 * Bump golang.org/x/crypto from 0.24.0 to 0.31.0 ([#2006](#2006)).
 * Bump golang.org/x/crypto from 0.30.0 to 0.31.0 in /bundle/internal/tf/codegen ([#2005](#2005)).
andrewnester added a commit that referenced this pull request Dec 18, 2024
Bundles:
* Allow overriding compute for non-development mode targets
([#1899](#1899)).
* Show an error when using a cluster override with 'mode: production'
([#1994](#1994)).

API Changes:
 * Added `databricks account federation-policy` command group.
* Added `databricks account service-principal-federation-policy` command
group.
* Added `databricks aibi-dashboard-embedding-access-policy delete`
command.
* Added `databricks aibi-dashboard-embedding-approved-domains delete`
command.

OpenAPI commit a6a317df8327c9b1e5cb59a03a42ffa2aabeef6d (2024-12-16)
Dependency updates:
* Upgrade TF provider to 1.62.0
([#2030](#2030)).
* Upgrade Go SDK to 0.54.0
([#2029](#2029)).
* Bump TF codegen dependencies to latest
([#1961](#1961)).
* Bump golang.org/x/term from 0.26.0 to 0.27.0
([#1983](#1983)).
* Bump golang.org/x/sync from 0.9.0 to 0.10.0
([#1984](#1984)).
* Bump github.com/databricks/databricks-sdk-go from 0.52.0 to 0.53.0
([#1985](#1985)).
* Bump golang.org/x/crypto from 0.24.0 to 0.31.0
([#2006](#2006)).
* Bump golang.org/x/crypto from 0.30.0 to 0.31.0 in
/bundle/internal/tf/codegen
([#2005](#2005)).
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.

4 participants