Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
d7a6df2 to
e5f9f52
Compare
|
Please rebase and and solve conflicts |
| "fs-extra", | ||
| "glob", | ||
| "google-sql-syntax-ts", | ||
| "ignore", |
There was a problem hiding this comment.
yarn.lock update is missing
There was a problem hiding this comment.
ignore@^5.2.0 already has an entry in yarn.lock (resolving to 5.3.2) because other packages depend on it. The range I declared matches that entry exactly, so yarn has nothing to add. I checked by running yarn install from scratch on the rebased branch: the lockfile came out byte-identical, and the yarn --frozen-lockfile step in ./scripts/lint passes. I also moved ignore into the new dependencies section added in #2338, and the generated @dataform/cli package.json now lists it. If you'd prefer a pinned version, like other recent dependency changes, I'm happy to switch.
| */ | ||
| export function buildProjectCopyFilter(resolvedProjectPath: string): (src: string) => boolean { | ||
| const ig = ignore(); | ||
| const gitignorePath = path.join(resolvedProjectPath, ".gitignore"); |
There was a problem hiding this comment.
Supporting .gitignore by default is the right step for 99% of users. We could add a new support to a .dataformignore file (similar to .dockerignore / .eslintignore). If .dataformignore exists, it could either override or supplement .gitignore (allowing users to un-ignore paths specifically for Dataform).
There was a problem hiding this comment.
Good idea, and it addresses the one real downside of this PR, so it's done here. .dataformignore supplements .gitignore: it's read second into the same matcher, so it can exclude more paths or un-ignore gitignored ones with !pattern. I chose supplement over override so users don't have to copy their whole .gitignore to change one path.
A few details, all documented and tested:
- As in git, a file can't be re-included while its parent directory is still excluded, so users un-ignore the directory (
!definitions/generated/) rather than a single file inside it. - Neither file can un-ignore
.gitornode_modules, andworkflow_settings.yamlis always copied, since compilation can't run without it. - Pattern matching follows the filesystem's case sensitivity, as git does with
core.ignorecase. Theignorelibrary ignores case by default, which on Linux would have letdefinitions/staging/silently dropdefinitions/Staging/*.sqlx. - Patterns are evaluated without consulting git's index, so a file that git tracks despite matching a pattern (e.g. via
git add -f) is still excluded.
| if (compileConfig.verbose) { | ||
| print(`Using isolated environment for @dataform/core@${workflowSettingsDataformCoreVersion}\n`); | ||
| print(`Copying project to temporary directory: ${temporaryProjectPath}\n`); | ||
| print(`Excluding .git, node_modules, and paths matched by the project's .gitignore\n`); |
There was a problem hiding this comment.
If the project has no .gitignore, this log message still claims it is excluding paths from .gitignore. A minor conditional check (hasGitignore ? ... : ...) would be cleaner.
There was a problem hiding this comment.
Fixed. The verbose log now names the ignore files it actually found, e.g. "paths matched by: .gitignore, .dataformignore". When there are none, it says "Excluding .git and node_modules (no .gitignore or .dataformignore in project root)".
Fixes dataform-co#2269. When dataformCoreVersion is set in workflow_settings.yaml, compile() copies the whole project directory to a temporary directory before running `npm i` there. That copy had no filter, so its cost scaled with everything under the project root -- most commonly a large .venv or build-output directory sitting next to definitions/ -- rather than with the Dataform project itself. Adds buildProjectCopyFilter(), an fs-extra copySync filter built from the project's own root .gitignore (already written by `dataform init`, but not previously consulted anywhere in the compile pipeline), plus a small always-ignored floor of .git and node_modules that a .gitignore negation pattern cannot override. Using the project's .gitignore rather than a hardcoded list of directory names avoids having to enumerate every ecosystem's junk directories (.venv, target/, __pycache__/, vendor/, coverage/, ...), since a project's .gitignore already states what that project treats as disposable. Behavior change worth calling out in review: a gitignored file is no longer copied, so it is also no longer compiled. A project that generates definitions into a gitignored path now has to unignore that path. Verbose mode prints which exclusions are applied. Projects with no .gitignore are unaffected beyond the .git/node_modules floor. No lockfile regeneration is needed: `ignore` is already resolved in yarn.lock at 5.3.2 as an existing transitive dependency, and the declared ^5.2.0 range matches that entry.
- Read an optional .dataformignore from the project root after the .gitignore, into the same ignore instance. It can exclude further paths, or un-ignore gitignored ones with `!pattern` (for example, definitions generated into a gitignored directory). The .git and node_modules floor still cannot be overridden by either file. - The verbose log now names the ignore files actually found, and no longer mentions a .gitignore when the project has none. - Reformat the new files with the repo's Prettier 3 config.
e5f9f52 to
e261524
Compare
- ignore() matches case-insensitively by default, so on a case-sensitive filesystem a `definitions/staging/` pattern also dropped `definitions/Staging/table.sqlx`, which git keeps. Always match case-sensitively: at worst this copies a little extra on case-insensitive filesystems, and never drops a definition. - Document that, as in git, a file can't be re-included while an ancestor directory is still excluded, with a concrete example, and test the child-only negation case.
- Compare the always-ignored .git/node_modules names case-insensitively, so .GIT or NODE_MODULES on a case-insensitive filesystem is excluded. - Always copy the root workflow_settings.yaml. compile() has already read it from the original project, and compilation can't proceed without it, so a broad pattern like `*.yaml` must not drop it. - Skip a .gitignore/.dataformignore that isn't a regular file, instead of failing the compile with EISDIR. - Document that patterns are evaluated without consulting git's index, so force-added tracked files matching a pattern are still excluded.
A fixed case policy was wrong in both directions: always-sensitive matching let a mixed-case negation miss its file on a case-insensitive filesystem, and always-folding the .git/node_modules floor excluded a legitimate NODE_MODULES directory on Linux. Detect once whether the project directory is case-insensitive (probing an existing entry under its case-swapped name, falling back to the platform default), and apply that to the ignore matcher, the always-ignored floor and the workflow_settings.yaml allowlist, as git does with core.ignorecase. Tests pin each mode explicitly so they behave the same on every platform.
|
Rebased onto main and conflicts resolved. The only conflict was |
3981f2b to
2a61411
Compare
Fixes #2269.
Per @kolina's comment on the issue ("Approach with
.gitignoreseems more effective"), this takes the.gitignore-aware route rather than a hardcoded exclude list. Following review, it also supports an optional.dataformignore.The problem
When
dataformCoreVersionis set inworkflow_settings.yaml,compile()copies the whole project directory to a temp dir before runningnpm ithere:No
filter, so the copy's cost scales with everything under the project root rather than with the Dataform project itself. In a mixed-tooling repo, a Python.venvsitting next todefinitions/gets copied byte-for-byte on everycompile.The change
Adds
buildProjectCopyFilter(), anfs-extracopySyncfilter built from:.gitignore, if present.dataform initalready writes one, but nothing in the compile pipeline consulted it until now..dataformignore, in the same syntax, read after.gitignoreinto the same matcher. It can exclude further paths, or un-ignore gitignored ones with!pattern, e.g.!definitions/generated/for definitions generated into a gitignored directory..gitandnode_modulesare always excluded, and the rootworkflow_settings.yamlis always copied, sincecompile()has already read it and compilation can't proceed without it.Using the project's
.gitignoreavoids having to enumerate every ecosystem's junk directories (.venv,target/,__pycache__/,vendor/,coverage/, ...). A project's.gitignorealready states what that project treats as disposable.The stateless-install guarantee is unchanged: core is still freshly installed into the temp dir on every compile.
Behavior change
An ignored file is no longer copied, so it is also no longer compiled. A project that generates definitions into a gitignored path needs to un-ignore that path, which
.dataformignorenow allows without touching.gitignore. As in git, a file can't be re-included while its parent directory is still excluded, so the directory itself must be un-ignored.Two things soften it:
.git/node_modulesexclusion.Notes on the implementation
core.ignorecase. The filter probes whether the project directory is case-insensitive (looking up an existing entry under its case-swapped name, falling back to the platform default) and applies that to pattern matching, negations, and both fixed rules.ignorematches case-insensitively by default, which on Linux would have letdefinitions/staging/silently dropdefinitions/Staging/*.sqlx.lstatSync, notstatSync, classifies directories for trailing-slash patterns like.venv/.statSyncfollows symlinks and throwsENOENTon a dangling one, which is a perfectly copyable input.lstatalso matches whatcopySyncitself does by default..gitignorefiles,.git/info/excludeand the user's global excludes are not consulted, so this under-excludes relative togit statusrather than over-excluding. A.gitignore/.dataformignorethat isn't a regular file is skipped.git add -f) is still excluded. Documented in the code.node_modulescannot actually be present on this path, sincecompile()rejects the project before copying if it finds one. That fixed exclusion therefore covers nested ones.ignore@^5.2.0already resolves to 5.3.2 inyarn.lockas an existing transitive dependency; a cleanyarn installleaves the lockfile byte-identical.ignoreis declared underdependenciesand listed in the CLI bundle's externals.Testing
19 unit tests in
cli/api/commands/compile_copy_filter_test.ts, covering.gitignoreand.dataformignoreseparately and together, un-ignoring (including the parent-directory limit), the fixed rules against negations, both case modes (tests pin the mode explicitly so they behave the same on every platform) plus the detection itself, dangling symlinks, in-project paths beginning with two dots, ignore-file names that are directories, and integration-style tests that run a realcopySync../scripts/lintis clean, including itsyarn --frozen-lockfilestep.Case-insensitive detection has only been exercised on Linux; the case-insensitive code paths are covered by the forced-mode tests.
🤖 Generated with Claude Code