Decouple repository root resolver from internal logic #2957
Conversation
… and update options in rally and stream runners
| // check the file exists | ||
| if _, err := repoRoot.Stat(includedFilePathRelFromRoot); err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
Variable includedFilePathRelFromRoot could be also a relative path outside repoRoot (something like ../path).
Would that fail repoRoot.Stat() in that case?
There was a problem hiding this comment.
the included file can be outside the repo root? i thought all files should be within the repo..
includedFilePath is the absolute path for the included file, taking into account the join of the relative path described inside the link file and the workdir.
includedFilePathRelFromRoot is then the relative from root, if this is outside root, repoRoot.Stat will fail, as it wont be valid? i would need confirmation of this. Right now is restricted to root
There was a problem hiding this comment.
All files referenced from link files should be within the repository, yes.
They can be outside the package, but always inside the repository.
…factor tests and added readme note
|
@mrodm i've updated the code with your feedback. i usually mark resolved the comments that have been addressed, please re-open if you find anything pending. thanks |
There was a problem hiding this comment.
It looks great, it seems to solve links resolution and does a good refactor to allow injection of the path to the root repository.
I did some tests and found some issues, commented in the files, though maybe some of them can be solved in follow ups.
Many comments are nitpicking, no need to do anything about them if you prefer current code.
cmd/benchmark.go
Outdated
| repoRoot, err := files.FindRepositoryRoot() | ||
| if err != nil { | ||
| return fmt.Errorf("locating repository root failed: %w", err) | ||
| } |
There was a problem hiding this comment.
Food for thought. We are adding more dependencies to the repository root, that is actually a dependency on having the package in a git repository.
It would be nice if it would be possible to use elastic-package with packages that are not in a repository [yet], or that are in a a repository, but from a different version control system.
But many subcommands already depend on this, so no need to take this into account now. It is good in any case to be more explicit about the dependencies of each command.
There was a problem hiding this comment.
We are adding more dependencies to the repository root
These changes have moved this dependency up to the initial call of any command that required it. I don't think the changes add, but identify better the dependencies for now. If we which to not restrict the commands to be run under the repository, it will be easier to make this change when having the dependency identified
internal/resources/fleetpolicy.go
Outdated
| PackagePolicies []FleetPackagePolicy | ||
|
|
||
| // RepoPath is the root of the repository. | ||
| RepoRoot *os.Root |
There was a problem hiding this comment.
The root is needed when appending the FleetPackage https://github.com/teresaromero/elastic-package/blob/2797-detach-repo-root/internal/resources/fleetpolicy_test.go#L127
internal/files/linkedfiles.go
Outdated
| packageRoot, err := packages.FindPackageRootFrom(filepath.Join(root.Name(), fromDir)) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("finding package root failed: %w", err) | ||
| } |
There was a problem hiding this comment.
The elastic-package links list command fails now if executed out of a package, with:
Error: listing linked packages failed: finding package root failed: package root not found
I think the intention was to list all links when executed outside a package, though it doesn't look like previous implementation was either doing it.
There was a problem hiding this comment.
From what i see at the command description, the path where the command is executed is taken into account to look for the links. What should be the behavour?
I think the intention was to list all links when executed outside a package
How this should look like? to run the command under the /packages directory, what is the expected output?
There was a problem hiding this comment.
current release does not show any feedback when running the command under a directory that is not a package, should we handle this error and warn the user that the command belongs to a working directory of a package?
There was a problem hiding this comment.
I have tried to link a fields file that includes fields with external: ecs and elastic-package fails to resolve these fields when building the package, with errors like:
Error: building package failed: invalid content found in built zip package: found 7 validation errors:
1. file ".../github.com/elastic/integrations/build/packages/system-2.6.1.zip/data_stream/cpu/fields/agent.yml" is invalid: field cloud.account.id with external key defined ("ecs") but no _dev/build/build.yml found
I guess it is trying to read the build.yml file from the target directory instead of from the source directory. It only happens with linked files.
Not sure if the issue is introduced with the current PR, so we can handle this in a follow up, as you prefer.
There was a problem hiding this comment.
could you walk me through this test? i am not sure i understood correctly the operation here.
Create a .link file for ecs.yml of one package ?
…use value receiver instead of pointer
… handling in tests
💚 Build Succeeded
History
|
4 participants
Resolves #2797
This pull request refactors how repository root and links definitions files are discovered and used throughout the codebase, improving consistency, error handling, and test coverage. The main changes include passing the repository root explicitly to functions that need it, updating the logic for finding and reading the links definitions file, and enhancing license file handling during package builds. Comprehensive unit tests have been added for these new behaviors.
Repository root and links file handling:
build,lint, and links-related commands incmd/build.go,cmd/lint.go,cmd/links.go) now explicitly locate and pass the repository root usingfiles.FindRepositoryRoot(), and use this to find the links definitions file viadocs.LinksDefinitionsFilePath(repoRoot). This improves reliability and consistency in locating project resources. [1] [2] [3] [4] [5] [6]License file handling during build:
internal/builder/packages.gonow takes the repository root as a parameter and uses it to locate and copy the license file into the package, supporting overrides via environment variable and handling missing files gracefully. The helper functions have been refactored for clarity and improved error handling. [1] [2] [3] [4] [5]Links map logic and API changes:
internal/docs/links_map.gohas been refactored to use pointer receivers, provide clearer error handling, and accept the links file path explicitly. The logic for finding the links definitions file now checks environment variables first and falls back to the default file in the repository root, returning an empty string if not found. [1] [2] [3]Unit tests for new behaviors:
internal/builder/packages_test.goandinternal/docs/links_map_test.gocover repository license file discovery, license copying logic, and links file resolution under various scenarios (env var, missing file, default file, YAML parsing). [1] [2]Imports and cleanup: