Include a materialized copy of built-in templates #2146

pietern · 2025-01-14T19:25:25Z

Changes

Include a materialized copy of built-in templates as reference output.

This updates the output comparison logic to work against an output directory. The doComparison function now always works on real files. It can now tell apart non-existing files and empty files (e.g., the .gitkeep files in templates).

pietern · 2025-01-14T19:26:43Z

@denik I tried using testutil.ReplaceOutput instead of using the replacement context directly. This takes care of a UUID in the output. But it also replaces numbers with <NUMID> which is not valid JSON.

acceptance/acceptance_test.go

acceptance/bundle/init/default-python/output/my_default_python/.gitignore

denik · 2025-01-15T08:55:18Z

@denik I tried using testutil.ReplaceOutput instead of using the replacement context directly. This takes care of a UUID in the output. But it also replaces numbers with which is not valid JSON.

We could easily fix that with by using "<NUMID>" instead.

denik · 2025-01-15T08:56:25Z

I think it would be useful (could be a follow up) to also run "bundle validate" and "bundle deploy" and other commands on these, so bundle/init is not a great name, because it's not just init. bundle/templates ?

This reverts commit 36c08d3.

…output

## Changes Replacement was split between the type `ReplacementContext` and the `ReplaceOutput` function. The latter also ran a couple of regular expressions. This change consolidates them such that it is up to the caller to compose the set of replacements to use. This change is required to accommodate UUID replacement in #2146.

…output

pietern · 2025-01-16T15:46:07Z

#2166 makes sure that initialized templates are formatted per ruff.

shreyas-goenka · 2025-01-16T17:10:39Z

acceptance/acceptance_test.go

-			t.Logf("Overwriting: %s", pathExpected)
-			testutil.WriteFile(t, pathExpected, valueNew)
-		} else {
-			t.Logf("Removing: %s", pathExpected)


Why remove this? It's helpful to update golden test files when files are deleted.

Auto-removal functionality is quite helpful actually. I'd prefer that we keep it.

We can do so by changing readIfExists to return (contents string, exists bool) to distinguish between empty and missing.

I missed this nuance, thanks.

I updated the code to deal with all combinations of missing files in a single place.

Please re-review the changes to acceptance_test.go.

acceptance/acceptance_test.go

## Changes The materialized templates included in #2146 include Python code that we require to be formatted. Instead of running ruff as part of the testcase, we can enforce that all Python code in the repository is formatted. It won't be possible to have a passing acceptance test for template initialization with unformatted code.

…output

shreyas-goenka · 2025-01-17T14:26:01Z

acceptance/acceptance_test.go

 	}

 	if !errors.Is(err, os.ErrNotExist) {
 		t.Fatalf("%s: %s", path, err)
 	}
-	return []byte{}
+	return []byte{}, false
 }

 func CopyDir(src, dst string, inputs, outputs map[string]bool) error {


It would be nice to have a unit test for this since this is part of the acceptance test infra.

Yeah, I agree. The amount of logic here has grown. While the coverage is implied through passing tests, something could slip through.

acceptance/acceptance_test.go

denik · 2025-01-17T14:59:09Z

acceptance/acceptance_test.go

-		} else {
-			t.Logf("Removing: %s", pathExpected)
-			_ = os.Remove(pathExpected)
+func doComparison(t *testing.T, repls testdiff.ReplacementsContext, dirRef, dirNew, relPath string) {


nice, I like having all the possible cases in one place.

CLI: * Added text output templates for apps list and list-deployments ([#2175](#2175)). * Fix duplicate "apps" entry in help output ([#2191](#2191)). Bundles: * Allow yaml-anchors in schema ([#2200](#2200)). * Show an error when non-yaml files used in include section ([#2201](#2201)). * Set WorktreeRoot to sync root outside git repo ([#2197](#2197)). * fix: Detailed message for using source-linked deployment with file_path specified ([#2119](#2119)). * Allow using variables in enum fields ([#2199](#2199)). * Add experimental-jobs-as-code template ([#2177](#2177)). * Reading variables from file ([#2171](#2171)). * Fixed an apps message order and added output test ([#2174](#2174)). * Default to forward slash-separated paths for path translation ([#2145](#2145)). * Include a materialized copy of built-in templates ([#2146](#2146)).

pietern added 4 commits January 14, 2025 20:24

Include the materialized template as reference output

9c9933e

Run ruff

34197d6

Update ruff call

a52032a

Use ReplaceOutput function

36c08d3

pietern temporarily deployed to test-trigger-is January 14, 2025 19:25 — with GitHub Actions Inactive

pietern mentioned this pull request Jan 14, 2025

Format default-python template #2110

Merged

denik reviewed Jan 15, 2025

View reviewed changes

acceptance/acceptance_test.go Outdated Show resolved Hide resolved

denik reviewed Jan 15, 2025

View reviewed changes

acceptance/bundle/init/default-python/output/my_default_python/.gitignore Outdated Show resolved Hide resolved

pietern added 5 commits January 15, 2025 10:35

Update

c3c4dcd

Revert "Use ReplaceOutput function"

ddcf0ed

This reverts commit 36c08d3.

Update

6a5db98

Merge remote-tracking branch 'origin/main' into acceptance-init-with-…

6c3a1fb

…output

Update outputs

658458f

pietern mentioned this pull request Jan 15, 2025

Use regular expressions for testdiff replacements #2151

Merged

pietern added 3 commits January 15, 2025 12:15

Merge remote-tracking branch 'origin/main' into acceptance-init-with-…

5f0d8d3

…output

Include UUID replacement

cd754e5

Install ruff in CI

f306f9c

pietern temporarily deployed to test-trigger-is January 15, 2025 11:20 — with GitHub Actions Inactive

Configure ruff

885e73a

pietern temporarily deployed to test-trigger-is January 15, 2025 12:20 — with GitHub Actions Inactive

Include gitignored file

addfc7d

pietern temporarily deployed to test-trigger-is January 15, 2025 12:25 — with GitHub Actions Inactive

pietern added 2 commits January 16, 2025 16:15

Merge remote-tracking branch 'origin/main' into acceptance-init-with-…

0d4c318

…output

Update outputs

782733c

pietern mentioned this pull request Jan 16, 2025

Format Python code with ruff #2166

Merged

pietern temporarily deployed to test-trigger-is January 16, 2025 15:40 — with GitHub Actions Inactive

pietern force-pushed the acceptance-init-with-output branch from 3884a71 to f45b156 Compare January 16, 2025 15:45

pietern temporarily deployed to test-trigger-is January 16, 2025 15:45 — with GitHub Actions Inactive

pietern marked this pull request as ready for review January 16, 2025 15:46

pietern requested review from andrewnester and shreyas-goenka as code owners January 16, 2025 15:46

pietern changed the title ~~Include the materialized template as reference output~~ Include a materialized copy of built-in templates Jan 16, 2025

pietern requested a review from denik January 16, 2025 15:49

shreyas-goenka reviewed Jan 16, 2025

View reviewed changes

pietern added 3 commits January 17, 2025 11:57

Merge remote-tracking branch 'origin/main' into acceptance-init-with-…

8ae5934

…output

Always compare files

92482ce

Update

edcd726

pietern temporarily deployed to test-trigger-is January 17, 2025 13:24 — with GitHub Actions Inactive

Include ruff ignore for SQL notebook

695ddc7

pietern temporarily deployed to test-trigger-is January 17, 2025 13:27 — with GitHub Actions Inactive

Move ruff config

07ca895

pietern temporarily deployed to test-trigger-is January 17, 2025 13:31 — with GitHub Actions Inactive

pietern requested a review from shreyas-goenka January 17, 2025 13:35

shreyas-goenka reviewed Jan 17, 2025

View reviewed changes

denik reviewed Jan 17, 2025

View reviewed changes

denik approved these changes Jan 17, 2025

View reviewed changes

shreyas-goenka approved these changes Jan 17, 2025

View reviewed changes

pietern added this pull request to the merge queue Jan 17, 2025

Merged via the queue into main with commit 50f6269 Jan 17, 2025
9 checks passed

pietern deleted the acceptance-init-with-output branch January 17, 2025 15:09

andrewnester mentioned this pull request Jan 23, 2025

[Release] Release v0.239.1 #2218

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Include a materialized copy of built-in templates #2146

Include a materialized copy of built-in templates #2146

pietern commented Jan 14, 2025 •

edited

Loading

pietern commented Jan 14, 2025

denik commented Jan 15, 2025 •

edited

Loading

denik commented Jan 15, 2025

pietern commented Jan 16, 2025

shreyas-goenka Jan 16, 2025

denik Jan 16, 2025

pietern Jan 17, 2025

shreyas-goenka Jan 17, 2025

pietern Jan 17, 2025

denik Jan 17, 2025

Include a materialized copy of built-in templates #2146

Include a materialized copy of built-in templates #2146

Conversation

pietern commented Jan 14, 2025 • edited Loading

Changes

pietern commented Jan 14, 2025

denik commented Jan 15, 2025 • edited Loading

denik commented Jan 15, 2025

pietern commented Jan 16, 2025

shreyas-goenka Jan 16, 2025

Choose a reason for hiding this comment

denik Jan 16, 2025

Choose a reason for hiding this comment

pietern Jan 17, 2025

Choose a reason for hiding this comment

shreyas-goenka Jan 17, 2025

Choose a reason for hiding this comment

pietern Jan 17, 2025

Choose a reason for hiding this comment

denik Jan 17, 2025

Choose a reason for hiding this comment

pietern commented Jan 14, 2025 •

edited

Loading

denik commented Jan 15, 2025 •

edited

Loading