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

Fix template initialization when running on Databricks #1912

Merged
merged 1 commit into from
Nov 20, 2024

Conversation

pietern
Copy link
Contributor

@pietern pietern commented Nov 18, 2024

Changes

When running the CLI on Databricks Runtime (DBR), use the extension-aware filer to write an instantiated template if the instance path is located in the workspace filesystem.

Notebooks cannot be written through the workspace filesystem's FUSE mount. As a result, this is the only method for initializing templates that contain notebooks when running the CLI on DBR and writing to the workspace filesystem.

Depends on #1910 and #1911.

Supersedes #1744.

Tests

  • Manually confirmed I can initialize a template with notebooks when running the CLI from the web terminal.

@pietern pietern force-pushed the template-wsfs-filer branch from 478b0eb to 16e5e78 Compare November 19, 2024 15:55
@pietern pietern force-pushed the template-write-filer branch from cf84dec to ce5df8f Compare November 20, 2024 09:52
Base automatically changed from template-write-filer to main November 20, 2024 10:11
Copy link

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: 1912
  • Commit SHA: 0a7e0180ffa8b68532f3d9537b363677ced27599

Checks will be approved automatically on success.

@eng-dev-ecosystem-bot
Copy link
Collaborator

Test Details: go/deco-tests/11931449688

@pietern pietern enabled auto-merge November 20, 2024 10:26
@pietern pietern added this pull request to the merge queue Nov 20, 2024
Merged via the queue into main with commit 886e149 Nov 20, 2024
10 checks passed
@pietern pietern deleted the template-wsfs-filer branch November 20, 2024 11:48
pietern added a commit that referenced this pull request Nov 20, 2024
**Note:** the `bundle generate` command now uses the `.<resource-type>.yml`
sub-extension for the configuration files it writes. Existing configuration
files that do not use this sub-extension are renamed to include it.

Bundles:
 * Make `TableName` field part of quality monitor schema ([#1903](#1903)).
 * Do not prepend paths starting with ~ or variable reference ([#1905](#1905)).
 * Fix workspace extensions filer accidentally reading notebooks ([#1891](#1891)).
 * Fix template initialization when running on Databricks ([#1912](#1912)).
 * Source-linked deployments for bundles in the workspace ([#1884](#1884)).
 * Added integration test to deploy bundle to /Shared root path ([#1914](#1914)).
 * Update filenames used by bundle generate to use `.<resource-type>.yml` ([#1901](#1901)).

Internal:
 * Extract functionality to detect if the CLI is running on DBR ([#1889](#1889)).
 * Consolidate test helpers for `io/fs` ([#1906](#1906)).
 * Use `fs.FS` interface to read template ([#1910](#1910)).
 * Use `filer.Filer` to write template instantiation ([#1911](#1911)).
github-merge-queue bot pushed a commit that referenced this pull request Nov 20, 2024
**Note:** the `bundle generate` command now uses the
`.<resource-type>.yml`
sub-extension for the configuration files it writes. Existing
configuration
files that do not use this sub-extension are renamed to include it.

Bundles:
* Make `TableName` field part of quality monitor schema
([#1903](#1903)).
* Do not prepend paths starting with ~ or variable reference
([#1905](#1905)).
* Fix workspace extensions filer accidentally reading notebooks
([#1891](#1891)).
* Fix template initialization when running on Databricks
([#1912](#1912)).
* Source-linked deployments for bundles in the workspace
([#1884](#1884)).
* Added integration test to deploy bundle to /Shared root path
([#1914](#1914)).
* Update filenames used by bundle generate to use `.<resource-type>.yml`
([#1901](#1901)).

Internal:
* Extract functionality to detect if the CLI is running on DBR
([#1889](#1889)).
* Consolidate test helpers for `io/fs`
([#1906](#1906)).
* Use `fs.FS` interface to read template
([#1910](#1910)).
* Use `filer.Filer` to write template instantiation
([#1911](#1911)).
pietern added a commit that referenced this pull request Jan 2, 2025
This used to work because the permission bits for builtin templates were
hardcoded to 0644 for files and 0755 for directories.

As of #1912 (and the PRs it depends on), builtin templates are no longer
pre-materialized to a temporary directory and read directly from the embedded
filesystem. It turns out that this builtin filesystem returns 0444 as the
permission bits for the files it contains. These bits are carried over to the
destination filesystem.

This change updates template materialization to always set the owner's write
bit. It doesn't really make sense to write read-only files and expect users to
work with these files in a VCS (note: Git only stores the executable bit).

The regression shipped as part of v0.235.0 and will be fixed as of v0.238.0.
github-merge-queue bot pushed a commit that referenced this pull request Jan 8, 2025
…2068)

## Changes

This used to work because the permission bits for built-in templates
were hardcoded to 0644 for files and 0755 for directories.

As of #1912 (and the PRs it depends on), built-in templates are no
longer pre-materialized to a temporary directory and read directly from
the embedded filesystem. This built-in filesystem returns 0444 as the
permission bits for the files it contains. These bits are carried over
to the destination filesystem.

This change updates template materialization to always set the owner's
write bit. It doesn't really make sense to write read-only files and
expect users to work with these files in a VCS (note: Git only stores
the executable bit).

The regression shipped as part of v0.235.0 and will be fixed as of
v0.238.0.

## Tests

Unit tests.
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