[CI] Support package discovery in nested directories - WIP#18495
[CI] Support package discovery in nested directories - WIP#18495mrodm wants to merge 17 commits intoelastic:mainfrom
Conversation
- Add ListPackages function to dev/citools/packages.go that walks a directory and returns sorted paths of all valid packages found. - Extend packageManifest struct with FormatVersion, Type and Version fields, and add IsValid() method mirroring dev/packagenames logic. - Refactor dev/packagenames to delegate package discovery to citools.ListPackages, removing the now-redundant walkPackagePaths. Generated with Claude (claude-sonnet-4-6)
- Rename readPackageManifest to ReadPackageManifest so it can be used from other packages. - Remove dev/packagenames/manifest.go and its local packageManifest type, replacing all usages with citools.ReadPackageManifest. - Update all internal callers (kibana.go, logsdb.go, subscription.go, packages.go) to use the renamed function. Generated with Claude (claude-sonnet-4-6)
- Replace the inline WalkDir loop in validatePackages with a call to citools.ListPackages, removing duplicated package-discovery logic. - Update testdata manifests (devexp, package_1) with the required format_version, name, type and version fields so they are recognised as valid packages by citools.ListPackages/IsValid. Generated with Claude (claude-sonnet-4-6)
- Add nested_packages testdata with packages at two directory levels: one at the top level and two under a category/ subdirectory, one of which has data streams. - Add five new TestValidatePackages cases covering: all packages with explicit owners, a missing owner for a nested package, category-level ownership inherited by nested packages, valid per-stream ownership in a nested package, and missing stream owner in a nested package. Generated with Claude (claude-sonnet-4-6)
Add a ListPackages target that calls citools.ListPackages to print all package paths found under the packages directory, one per line. Generated with Claude (claude-sonnet-4-6)
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| list_all_directories() { | ||
| find . -maxdepth 1 -mindepth 1 -type d | xargs -I {} basename {} | sort | ||
| mage listPackages | grep -E '^packages/(elastic_package_registry|nginx)$' | ||
| } |
There was a problem hiding this comment.
🔴 Critical scripts/common.sh:831
list_all_directories() now hardcodes a grep filter that returns only two package names (elastic_package_registry and nginx). Since callers trigger_integrations_in_parallel.sh and test_integrations_with_serverless.sh iterate over this output to drive CI pipelines, all other packages are silently excluded from testing and publishing. If this restriction is intentional, consider documenting why only these two packages should run; otherwise, the filter should be removed to restore full package coverage.
list_all_directories() {
- mage listPackages | grep -E '^packages/(elastic_package_registry|nginx)$'
+ mage listPackages
}🤖 Copy this AI Prompt to have your agent fix this:
In file .buildkite/scripts/common.sh around lines 831-833:
`list_all_directories()` now hardcodes a grep filter that returns only two package names (`elastic_package_registry` and `nginx`). Since callers `trigger_integrations_in_parallel.sh` and `test_integrations_with_serverless.sh` iterate over this output to drive CI pipelines, all other packages are silently excluded from testing and publishing. If this restriction is intentional, consider documenting why only these two packages should run; otherwise, the filter should be removed to restore full package coverage.
There was a problem hiding this comment.
This will be removed before merging the PR. This grep is there to test just a subset of packages instead of the ~400 packages.
- Add -d "${WORKSPACE}" to the mage call in list_all_directories so it
always finds magefile.go regardless of the working directory when invoked.
- Remove pushd packages / popd wrappers in trigger_integrations_in_parallel.sh
and test_integrations_with_serverless.sh since list_all_directories now
returns full paths (e.g. packages/nginx) relative to the repo root.
- Fix process_package: the parameter rename from package to package_path was
incomplete — pushd, teardown calls, and log lines still referenced the
old ${package} variable (now unbound). Use ${package_path} for directory
operations and ${package_name} (from package_name_manifest) for labels.
- Update test_one_package.sh to derive parent_path and package_name from
the received package_path instead of assuming a packages/ prefix.
- Consistently rename local variables from package to package_name across
check_package, build_zip_package, teardown_*_package, run_tests_package,
and related helpers.
Generated with Claude (claude-sonnet-4-6)
- Export manifestFileName as ManifestFileName in dev/citools/packages.go so it can be reused across packages. - Remove the duplicate manifestFileName constant from dev/packagenames and replace its usage with citools.ManifestFileName. - Replace the hardcoded "manifest.yml" string in dev/codeowners with citools.ManifestFileName. Generated with Claude (claude-sonnet-4-6)
Generated with Claude (claude-sonnet-4-6)
mrodm
force-pushed
the
allow-multiple-directories
branch
from
17332f9 to
e2454f2
Compare
Extract the pushd/is_pr_affected/popd pattern with error handling into a reusable should_test_package function, to be used from test_integrations_with_serverless.sh and trigger_integrations_in_parallel.sh. Generated with the assistance of Claude Sonnet 4.6
🚀 Benchmarks reportTo see the full report comment with |
Replace find-based flat directory loop with list_all_directories so that packages organised in subdirectories are discovered correctly. Also replace `cat manifest.yml | yq` with direct `yq manifest.yml` calls. Generated with the assistance of Claude Sonnet 4.6
|
|
||
| buildkite-agent pipeline upload "${PIPELINE_FILE}" | ||
| cat "${PIPELINE_FILE}" | ||
| # buildkite-agent pipeline upload "${PIPELINE_FILE}" |
There was a problem hiding this comment.
🔴 Critical scripts/build_packages.sh:159
The buildkite-agent pipeline upload command is replaced with cat, so the pipeline that signs and publishes packages is never uploaded to Buildkite. Built packages will remain in artifacts but never reach the package registry.
cat "${PIPELINE_FILE}"
-# buildkite-agent pipeline upload "${PIPELINE_FILE}"
+buildkite-agent pipeline upload "${PIPELINE_FILE}"🤖 Copy this AI Prompt to have your agent fix this:
In file .buildkite/scripts/build_packages.sh around lines 159-161:
The `buildkite-agent pipeline upload` command is replaced with `cat`, so the pipeline that signs and publishes packages is never uploaded to Buildkite. Built packages will remain in artifacts but never reach the package registry.
There was a problem hiding this comment.
This is a change to allow debugging the script safely. This will be reverted before merging the PR.
mrodm
force-pushed
the
allow-multiple-directories
branch
from
c280270 to
71e903e
Compare
| if skipPublishing ; then | ||
| echo "packageStoragePublish: not the main branch or a backport branch, nothing will be published" | ||
| exit 0 | ||
| # exit 0 | ||
| fi |
There was a problem hiding this comment.
🟡 Medium scripts/build_packages.sh:88
When skipPublishing returns true, the script prints "nothing will be published" but continues executing instead of exiting. This causes the script to proceed through expensive build steps (tool installation, package building, artifact copying, pipeline generation) on branches that should skip publishing entirely, wasting resources and potentially causing side effects.
if skipPublishing ; then
echo "packageStoragePublish: not the main branch or a backport branch, nothing will be published"
- # exit 0
+ exit 0
fi🤖 Copy this AI Prompt to have your agent fix this:
In file .buildkite/scripts/build_packages.sh around lines 88-91:
When `skipPublishing` returns true, the script prints "nothing will be published" but continues executing instead of exiting. This causes the script to proceed through expensive build steps (tool installation, package building, artifact copying, pipeline generation) on branches that should skip publishing entirely, wasting resources and potentially causing side effects.
There was a problem hiding this comment.
Changes related to debugging. It will be reverted before merging.
| SIGNING_STEP_KEY: "sign-service" | ||
| ARTIFACTS_FOLDER: "packageArtifacts" | ||
| DRY_RUN: "${DRY_RUN}" | ||
| DRY_RUN: "true" |
There was a problem hiding this comment.
🟡 Medium scripts/build_packages.sh:150
DRY_RUN is hardcoded to "true" on line 150, so the trigger_publish_packages.sh step always runs in dry-run mode regardless of the DRY_RUN environment variable value. Packages will never actually be published when the pipeline intends to run for real. Consider reverting to "${DRY_RUN}" to respect the environment variable.
| DRY_RUN: "true" | |
| DRY_RUN: "${DRY_RUN}" |
🤖 Copy this AI Prompt to have your agent fix this:
In file .buildkite/scripts/build_packages.sh around line 150:
`DRY_RUN` is hardcoded to `"true"` on line 150, so the `trigger_publish_packages.sh` step always runs in dry-run mode regardless of the `DRY_RUN` environment variable value. Packages will never actually be published when the pipeline intends to run for real. Consider reverting to `"${DRY_RUN}"` to respect the environment variable.
There was a problem hiding this comment.
Changes related to debugging. It will be reverted before merging.
Replace with_go with with_mage so that mage is available when list_all_directories is called. with_mage already calls with_go internally so Go setup is preserved. Generated with the assistance of Claude Sonnet 4.6
|
|
||
| if skipPublishing ; then | ||
| echo "packageStoragePublish: not the main branch or a backport branch, nothing will be published" | ||
| exit 0 | ||
| # exit 0 | ||
| fi | ||
|
|
There was a problem hiding this comment.
🟡 Medium scripts/build_packages.sh:87
When skipPublishing returns true, the script prints "nothing will be published" but continues executing instead of exiting. It proceeds to build packages, copy artifacts, and generate the signing/publishing pipeline YAML, wasting CI resources on non-main/non-backport branches. If the DRY_RUN or buildkite-agent pipeline upload guards are re-enabled later, packages could be signed and published from arbitrary feature branches.
@@ -88,5 +88,5 @@
if skipPublishing ; then
echo "packageStoragePublish: not the main branch or a backport branch, nothing will be published"
- # exit 0
+ exit 0
fi
add_bin_path🤖 Copy this AI Prompt to have your agent fix this:
In file .buildkite/scripts/build_packages.sh around lines 87-92:
When `skipPublishing` returns true, the script prints "nothing will be published" but continues executing instead of exiting. It proceeds to build packages, copy artifacts, and generate the signing/publishing pipeline YAML, wasting CI resources on non-main/non-backport branches. If the `DRY_RUN` or `buildkite-agent pipeline upload` guards are re-enabled later, packages could be signed and published from arbitrary feature branches.
There was a problem hiding this comment.
Changes related to debugging. It will be reverted before merging.
Add a termination check for ownerDir == "/" to prevent an infinite loop
when filepath.Dir reaches the filesystem root on non-Windows systems,
where filepath.Dir("/") returns "/" instead of ".".
Generated with the assistance of Claude Sonnet 4.6
| return nil | ||
| } | ||
|
|
||
| func (codeowners *githubOwners) findOwnerForFile(path string) ([]string, bool) { |
There was a problem hiding this comment.
🟢 Low codeowners/codeowners.go:203
The loop in findOwnerForFile terminates when ownerDir becomes . or / before checking the map for a root-level owner entry (key /). Files in subdirectories never match a root CODEOWNERS entry because the final parent directory is never searched.
🤖 Copy this AI Prompt to have your agent fix this:
In file dev/codeowners/codeowners.go around line 203:
The loop in `findOwnerForFile` terminates when `ownerDir` becomes `.` or `/` **before** checking the map for a root-level owner entry (key `/`). Files in subdirectories never match a root CODEOWNERS entry because the final parent directory is never searched.
Evidence trail:
dev/codeowners/codeowners.go lines 203-216 at REVIEWED_COMMIT: The function `findOwnerForFile` shows the loop structure where the break condition `if ownerDir == "." || ownerDir == "/"` is checked AFTER updating `ownerDir` but BEFORE the next iteration's map lookup. This means when `ownerDir` becomes `.` (for relative paths) or `/` (for absolute paths), the loop breaks without checking the map for the root-level owner entry.
| if filepath.IsAbs(path) { | ||
| path = strings.TrimPrefix(path, string(filepath.Separator)) | ||
| } |
There was a problem hiding this comment.
🟢 Low codeowners/codeowners.go:208
On Windows, findOwnerForFile with a path like C:\foo\bar produces incorrect lookup keys. filepath.IsAbs returns true, but strings.TrimPrefix with filepath.Separator (\) fails to trim C from C:\foo\bar, leaving the path unchanged. The subsequent loop then searches for keys like /C:/foo which will never match the expected codeowners entries (which use repository-relative paths). Consider using filepath.ToSlash on the full path and removing a leading / instead of relying on platform-specific separator behavior.
- if filepath.IsAbs(path) {
- path = strings.TrimPrefix(path, string(filepath.Separator))
- }
+ path = filepath.ToSlash(path)
+ if strings.HasPrefix(path, "/") {
+ path = strings.TrimPrefix(path, "/")
+ }🤖 Copy this AI Prompt to have your agent fix this:
In file dev/codeowners/codeowners.go around lines 208-210:
On Windows, `findOwnerForFile` with a path like `C:\foo\bar` produces incorrect lookup keys. `filepath.IsAbs` returns true, but `strings.TrimPrefix` with `filepath.Separator` (`\`) fails to trim `C` from `C:\foo\bar`, leaving the path unchanged. The subsequent loop then searches for keys like `/C:/foo` which will never match the expected codeowners entries (which use repository-relative paths). Consider using `filepath.ToSlash` on the full path and removing a leading `/` instead of relying on platform-specific separator behavior.
Evidence trail:
dev/codeowners/codeowners.go lines 203-222 at REVIEWED_COMMIT: The `findOwnerForFile` function at line 208-210 uses `strings.TrimPrefix(path, string(filepath.Separator))` to handle absolute paths. On Windows, `filepath.Separator` is `\`, but Windows absolute paths like `C:\foo\bar` start with a drive letter, not a separator. Go's `strings.TrimPrefix` only trims if the string starts with the prefix, so nothing is trimmed. Line 213 then constructs lookup keys with `/C:/foo` format which won't match repository-relative codeowners entries.
💚 Build Succeeded
History
cc @mrodm |
3 participants
Summary
This PR centralises package discovery logic into a new
citools.ListPackagesfunction and uses it acrosspackagenamesandcodeowners, replacing the previously duplicatedfilepath.WalkDirimplementations. As a side effect, both checks now support packages organised in nested directory structures (e.g.packages/category/my_package), not just flatpackages/my_packagelayouts.Proposed commit
WHAT:
Allow packages to be placed in nested subdirectories under
packages/(e.g.
packages/<category>/<name>) instead of being restricted to the flatpackages/<name>layout. Previously,codeownersdiscovery logic would silently ignore any package not sitting directly under
packages/, meaning nested packages would bypass ownership validation entirely.WHY:
As the number of integrations grows, being able to organise packages into
category subdirectories becomes necessary to keep the repository manageable.
The existing tooling hard-coded a single-level walk, blocking this. This change
makes nested directory layouts a supported and validated structure across all
CI tooling, so teams can start grouping packages without breaking checks.
Changes
dev/citoolsListPackages(dir string) ([]string, error)— walks a directory recursively, returns sorted paths of all valid packages found. A package is considered valid when itsmanifest.ymlcontains non-emptyformat_version,name,type(one ofintegration,input,content) andversionfields.packageManifeststruct withFormatVersion,TypeandVersionfields, and add anIsValid()method, mirroring the logic previously only present indev/packagenames.readPackageManifestasReadPackageManifestso it can be reused from other packages.dev/packagenamesmanifest.goand its localpackageManifesttype; replace all usages withcitools.ReadPackageManifest.walkPackagePathsfunction with a call tocitools.ListPackages.dev/codeownersfilepath.WalkDirloop invalidatePackageswith a call tocitools.ListPackages, removing the duplicate discovery logic.format_version,name,typeandversionfields.nested_packagestestdata fixture with packages at two directory levels (top-level and under acategory/subdirectory, one with data streams).TestValidatePackagescases covering: explicit per-package owners, missing owner for a nested package, category-level inherited ownership, valid per-stream ownership in a nested package, and missing stream owner in a nested package.magefile.goListPackagesmage target that prints all package paths under thepackages/directory, one per line..buildkite/scripts/common.shshould_test_packagehelper function that encapsulates the pushd/is_pr_affected/popdpattern along with fatal-error handling, replacing near-identical blocks that were duplicated intest_integrations_with_serverless.shandtrigger_integrations_in_parallel.sh`..buildkite/scripts/build_packages.shfind-based flat directory loop withlist_all_directories(backed bymage listPackages) so packages in nested subdirectories are discovered correctly.with_gowithwith_mage(which callswith_gointernally) somageis available whenlist_all_directoriesruns.Backport pipeline
PACKAGE_FOLDER_NAMEas a required input parameter frompipeline.backport.ymlandbackport_branch.sh. The package path is now resolved automatically by a newget_package_pathfunction that looks up the package whosemanifest.ymlnamefield matches the givenPACKAGE_NAME, usingmage listPackagesto iterate all discovered packages.docs/extend/developer-workflow-support-old-package.mdto reflect the removed parameter.Checklist
changelog.ymlfile.Author's Checklist
mage checkpasses locally with no errorsmage ListPackagescorrectly lists all packages,including any nested under a subdirectory
in nested directories correctly
list_all_directories..buildkite/scripts/build_packages.shHow to test this PR locally
Related issues