change package and analysis interface#57
Conversation
d99c18b to
3ea0726
Compare
Signed-off-by: gotwarlost <krishnan.anantheswaran@elastic.co>
There was a problem hiding this comment.
Pull request overview
This PR introduces a new directory-based packaging and analysis workflow for fn-hcl-tools, with optional composition.yaml metadata to support library files and XRD type information (backwards-incompatible change to the CLI interface and module assumptions).
Changes:
- Add
function/internal/compositionloader API to load config + enumerate top-level.hclfiles plus explicitlibraryFiles. - Update
fn-hcl-tools analyze/packagecommands to accept a directory argument (default.) instead of file lists. - Update examples and expected outputs to reference
main.hcl(vssrc/main.hcl) and add test coverage for the new directory/module behavior.
Reviewed changes
Copilot reviewed 33 out of 34 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| function/internal/composition/testdata/with-libs/main.hcl | Test fixture for module main file referencing a library function. |
| function/internal/composition/testdata/with-libs/lib/bar.hcl | Test fixture library function used by the module. |
| function/internal/composition/testdata/with-libs/composition.yaml | Test fixture config declaring XRD metadata and libraryFiles. |
| function/internal/composition/testdata/multi-hcl/b.hcl | Test fixture for multiple top-level HCL files packaging. |
| function/internal/composition/testdata/multi-hcl/a.hcl | Test fixture for multiple top-level HCL files packaging. |
| function/internal/composition/testdata/missing-lib/main.hcl | Test fixture to validate error handling for missing library file. |
| function/internal/composition/testdata/missing-lib/composition.yaml | Test fixture referencing a non-existent library file. |
| function/internal/composition/testdata/invalid-yaml-config/main.hcl | Test fixture used with invalid composition.yaml. |
| function/internal/composition/testdata/invalid-yaml-config/composition.yaml | Test fixture containing invalid YAML to trigger config parse errors. |
| function/internal/composition/testdata/invalid-hcl/main.hcl | Test fixture intentionally invalid HCL to trigger analysis failures. |
| function/internal/composition/testdata/dir-only/main.hcl | Test fixture for a single-file module in a directory. |
| function/internal/composition/testdata/dir-as-lib/main.hcl | Test fixture for “libraryFiles points at a directory” error case. |
| function/internal/composition/testdata/dir-as-lib/lib/.gitkeep | Placeholder to ensure the lib directory exists in the fixture. |
| function/internal/composition/testdata/dir-as-lib/composition.yaml | Test fixture where libraryFiles incorrectly references a directory. |
| function/internal/composition/os-fs.go | Adds an FS implementation backed by the OS for loader/packager. |
| function/internal/composition/composition_test.go | Adds unit tests covering directory-based packaging and analysis scenarios. |
| function/internal/composition/composition.go | Implements loader, config parsing, file enumeration, archive creation, and analysis execution. |
| function/internal/composition/api_test.go | Adds API-level tests for loading config and files (incl. XRD + libs). |
| function/internal/composition/api.go | Introduces the composition module public API (Load/Package/Analyze) and config schema. |
| function/example/user-function/composition.yaml | Updates inline txtar marker paths to main.hcl. |
| function/example/spec-example/composition.yaml | Updates inline txtar marker paths to main.hcl. |
| function/example/set-status/composition.yaml | Updates inline txtar marker paths to main.hcl. |
| function/example/set-status-incomplete/src/expected.yaml | Updates expected diagnostics/output paths from src/main.hcl to main.hcl. |
| function/example/set-status-incomplete/composition.yaml | Updates inline txtar marker paths to main.hcl. |
| function/example/set-context/composition.yaml | Updates inline txtar marker paths to main.hcl. |
| function/example/extra-resources-present/composition.yaml | Updates inline txtar marker paths to main.hcl. |
| function/example/extra-resources-absent/src/expected.yaml | Updates expected diagnostics/output paths from src/main.hcl to main.hcl. |
| function/example/extra-resources-absent/composition.yaml | Updates inline txtar marker paths to main.hcl. |
| function/example/basic-resource/composition.yaml | Updates inline txtar marker paths to main.hcl. |
| function/example/basic-resource-list/composition.yaml | Updates inline txtar marker paths to main.hcl. |
| function/example/basic-locals/composition.yaml | Updates inline txtar marker paths to main.hcl. |
| function/cmd/fn-hcl-tools/tools.go | Changes CLI package/analyze to accept a directory and delegates to composition package. |
| function/api/api.go | Exposes composition module loading types/functions via the public function/api package. |
| function/.scripts/gen-comp.sh | Updates script generation to call fn-hcl-tools package with a directory argument (src/). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: gotwarlost <krishnan.anantheswaran@elastic.co>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 33 out of 34 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: gotwarlost <krishnan.anantheswaran@elastic.co>
Signed-off-by: gotwarlost <krishnan.anantheswaran@elastic.co>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 33 out of 34 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: gotwarlost <krishnan.anantheswaran@elastic.co>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 33 out of 34 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: gotwarlost <krishnan.anantheswaran@elastic.co>
Signed-off-by: gotwarlost <krishnan.anantheswaran@elastic.co>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 38 out of 39 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| log.Printf("WARN: ignore metadata load error: %v", err) | ||
| cfg = &Config{} |
There was a problem hiding this comment.
When ignoreMetadataErrors is true, this uses the package-level log.Printf, which will include timestamps/prefixes by default (unlike doAnalyze, which uses log.New(os.Stderr, "", 0)). Consider using a consistent logger here (and in other metadata-warning logs) so CLI output stays stable and machine-parsable.
| errMsg := fmt.Sprintf("library file %q is an absolute path, not allowed", file) | ||
| if l.ignoreMetadataErrors { | ||
| log.Println(errMsg) | ||
| continue |
There was a problem hiding this comment.
These metadata-error branches log via the package-level logger (log.Println), which will include timestamps/prefixes by default. Prefer routing through a log.New(os.Stderr, "", 0) instance (or otherwise standardizing flags/prefix) to keep warnings consistent with other fn-hcl-tools output.
| // since the file list has file relative to the directory loaded | ||
| // we need to make it relative to the working directory instead. | ||
| contents, err := l.fs.ReadFile(filepath.Join(dir, file)) |
There was a problem hiding this comment.
This comment is misleading: fsFiles are relative to dir, and filepath.Join(dir, file) is constructing the path to read from (relative to the current working dir if dir is relative), not “making it relative to the working directory”. Consider rewording to describe why the join is needed (i.e., to resolve relative archive paths back to filesystem paths).
| (cd $dir && fn-hcl-tools package src/ >/tmp/script.txtar) | ||
| (cd $dir && cat src/comp-template.yaml | script="$(cat /tmp/script.txtar | jq -sR)" envsubst | yq -P>composition.yaml) |
There was a problem hiding this comment.
Shell variables should be quoted to avoid breakage if a directory name contains whitespace or glob characters. Quote $dir in the subshell cd command (and consider quoting other expansions in this script for consistency).
ydubreuil
left a comment
There was a problem hiding this comment.
LGTM
It's nice to be able to embed lib files, that will allow to stream packaging code and make the lib code work with the language server.
This PR changes the packaging and analysis interface of
fn-hcl-toolsin a backwards-incompatible way.This is to add some structure and assumptions to packaging such that the language server can make identical assumptions and work correctly.
Just like terraform,
.hclfiles are now only picked up from a single directory.The commands for packaging and analysis take a single directory name as argument instead of a bunch of file names which used to happen before.
In addition, the author can write a
composition.yamlfile in that directory which looks as follows:The YAML file allows for a couple of things:
$(pwd)The language server is still unaware of this tooling but this PR sets up the situation for making it work in a subsequent commit.
Parts of the API for the composition internal package is made public rather for language server use.