Skip to content

Include a materialized copy of built-in templates - #2146

Merged
pietern merged 22 commits into
mainfrom
acceptance-init-with-output
Jan 17, 2025
Merged

Include a materialized copy of built-in templates#2146
pietern merged 22 commits into
mainfrom
acceptance-init-with-output

Conversation

@pietern

@pieternpietern commented Jan 14, 2025

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
ContributorAuthor

@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.

Comment threadacceptance/acceptance_test.go Outdated
@denik

denik commented Jan 15, 2025

Copy link
Copy Markdown
Contributor

@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

Copy link
Copy Markdown
Contributor

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 ?

pietern added a commit that referenced this pull request Jan 15, 2025
## 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.
@pieternpietern changed the title Include the materialized template as reference outputInclude a materialized copy of built-in templatesJan 16, 2025
@pietern
pietern requested a review from denikJanuary 16, 2025 15:49
t.Logf("Overwriting: %s", pathExpected)
testutil.WriteFile(t, pathExpected, valueNew)
} else {
t.Logf("Removing: %s", pathExpected)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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.

Comment threadacceptance/acceptance_test.go
Comment threadacceptance/acceptance_test.go Outdated
github-merge-queueBot pushed a commit that referenced this pull request Jan 17, 2025
## 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.
return []byte{}, false
}

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@pietern
pietern added this pull request to the merge queueJan 17, 2025
Merged via the queue into main with commit 50f6269Jan 17, 2025
@pietern
pietern deleted the acceptance-init-with-output branch January 17, 2025 15:09
andrewnester added a commit that referenced this pull request Jan 23, 2025
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)).
github-merge-queueBot pushed a commit that referenced this pull request Jan 23, 2025
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)).
denik pushed a commit that referenced this pull request May 20, 2026
## 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.
denik pushed a commit that referenced this pull request May 20, 2026
## 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.
denik pushed a commit that referenced this pull request May 20, 2026
## 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).
denik pushed a commit that referenced this pull request May 20, 2026
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)).
Sign up for freeto 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.

3 participants

@pietern@denik@shreyas-goenka