feat: add elastic-package requires update command to bump requires pins from the registry#3593
Conversation
…encies Introduced a new command `elastic-package requires` to manage dependencies for integration packages. This includes the ability to update `requires.input` and `requires.content` versions from the package registry, ensuring compatibility with the integration package's Kibana version constraint. The command supports a dry-run option and outputs results in both table and JSON formats. Additionally, updated the README and documentation to reflect these changes, including usage examples for the new command.
Updated the `latestRevisionNewerThan` function to remove error handling, returning a single value instead. This change streamlines the code in `updates.go`. Minor formatting adjustments were also made in `requires.go` for consistency in output formatting.
…yVersion Updated the function name from `SetRequiresDependencyVersion` to `setRequiresDependencyVersion` for consistency with Go naming conventions. Adjusted all references in the test and implementation files accordingly. This change enhances code readability and maintains uniformity in function naming.
Removed the `requires_test.go` file and integrated its tests into the new `requiresupdates` package. Introduced new files for managing compatibility checks and manifest updates, enhancing the structure and maintainability of the codebase. This change improves the organization of dependency management for integration packages, ensuring better clarity and separation of concerns.
Consolidated logging for skipped packages in the requires command, replacing direct stderr output with logger warnings for better consistency. Updated the sorting mechanism in the dependency resolution to utilize the `slices` package, enhancing performance and readability. Minor formatting adjustments were also made for improved clarity in output.
Introduced new test cases in `updates_test.go` to validate the behavior of content dependencies during updates. The tests cover scenarios for exact pin bumps, constraint style bumps, and cases where no update is needed. Additionally, refactored the dependency resolution logic in `updates.go` to support parsing of current versions as either exact semver or constraints, enhancing the accuracy of version bump detection.
Removed the exact pinning option from the version replacement functions to streamline the logic for updating version lines in manifests. The changes ensure that the existing quote style is preserved during updates. Additionally, updated related test cases to reflect the new behavior and added tests for various version update scenarios, enhancing coverage and reliability.
Introduced several unit tests for the `latestRevision` function in `updates.go` to validate its behavior with various input scenarios. Tests cover cases for empty input, single revision, unsorted revisions, and handling of unparseable versions. This enhances test coverage and ensures the function correctly identifies the highest semantic version from the provided package manifests.
Added new test cases in `compatibility_test.go` to validate behavior for strict-greater lower bounds in Kibana version constraints. The tests ensure that the logic correctly identifies valid versions within specified ranges, improving coverage and accuracy of version constraint handling.
Replace manual profile flag parsing and LoadProfile call with the shared cobraext.GetProfileFlag helper, consistent with testrunner and benchmark commands. Also gains ErrNotAProfile handling with a human-readable list of valid profiles. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
requiresBold was an exact duplicate of bold already declared in the same cmd package. Replace all uses and drop the redundant declaration and color import. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…date printRequiresUpdateResult already handles SkipReason internally, and when it is set there are no proposals so the warning loop is always a no-op. The early return only duplicated the print call unnecessarily. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Extract kibanaBumpWarning helper to collapse three near-identical warning-building blocks in resolveDependency, and introduce latestRevisionWhere to unify the shared loop across three functions. Remove spurious error return from latestRevision. Consolidate TestLatestRevision subtests under a single table-driven test. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Extract isVersionOutdated as a package-level function so the outdated check is testable and searchable. Replace char-by-char whitespace loops in replaceVersionOnLine with strings.TrimLeft/TrimRight. Convert TestKibanaConstraintsOverlap to table-driven subtests for independent failure isolation. Remove stale source line reference from test comment. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…date When --format json is used, return after encoding so no plain-text messages appear on stdout alongside the JSON. Also guard "No dependencies to update" behind result.SkipReason == "" so it doesn't fire redundantly when the skip reason is already present in the output. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ived candidates Remove representativeKibanaVersions and kibanaVersionsSatisfyingIntegration. candidateKibanaVersions now extracts literal versions from both the integration and dependency constraints, so each side's boundaries probe the other naturally. Adds test coverage for empty dependency kibana constraint. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…es-update-command
| "format", | ||
| "install", | ||
| "lint", | ||
| "requires", |
There was a problem hiding this comment.
not sure this will be used on the automated script but i believe is worth enabling it for local use
| result = append(result, v) | ||
| } | ||
| for _, constraint := range constraints { | ||
| for _, branch := range strings.Split(constraint, "||") { |
There was a problem hiding this comment.
This assumes that constraints are somehow ordered, isn't it?
conditions:
kibana:
version: "^8.0.0 || ^9.0.0"Would it work successfully with this other constraint ?
conditions:
kibana:
version: "^9.0.0 || ^8.0.0"Based on the minVersionFromConstraintBranch function.
Not sure if this is happening now in the current packages.
There was a problem hiding this comment.
The new subset check evaluates each || branch independently, so order does not affect the result. Both orderings are covered by the OR ordering invariant test cases in compatibility_test.go.
| vStr := fmt.Sprintf("kibana.version=%s", v.Original()) | ||
| if packages.CheckConditions(integManifest, []string{vStr}) == nil && | ||
| packages.CheckConditions(depManifest, []string{vStr}) == nil { | ||
| return true, nil | ||
| } |
There was a problem hiding this comment.
I'm not sure if this is doing what we expect (or what I think I would expect).
For instance, an integration package with this kibana constraint:
conditions:
kibana:
version: "~9.1.0 || ^9.4.0""and a new input package dependency (set as required) update with kibana constraint:
conditions:
kibana:
version: "^9.4.0""Would return true in package.CheckConditions because the constraint is validated with the part of the constraint ^9.4.0. However, that integration package supports some Elastic stacks that this new input package would not be valid (9.1.x).
And this could be even more complicated to calculated if both packages contain constraints with || or ~ instead of ^.
Example of a package with several constraints with OR:
https://github.com/elastic/integrations/blob/cc8af15ae202a23ee52f5099793c40cce3ce50f2/packages/microsoft_sentinel/manifest.yml#L13
There are also opened constrains (that I guess they would be less problematic here):
https://github.com/elastic/integrations/blob/cc8af15ae202a23ee52f5099793c40cce3ce50f2/packages/cloud_asset_inventory/manifest.yml#L17
I'm not sure about the approach to take here. @jsoriano could you please chime in here too?
There was a problem hiding this comment.
Yes, I think we need to reconsider the algorithm here. What we really would need to check is that the integration constraint is a subset of the dependency constraint. Added some comments about this in the tests.
There was a problem hiding this comment.
Replaced the overlap check with a subset check. The algorithm splits the integration constraint on || and probes each branch with three representative versions (floor, floor+1 patch, major ceiling) against the dependency constraint. If any probe satisfies the branch but not the dependency, it returns false.
jsoriano
left a comment
There was a problem hiding this comment.
The high-level approach looks good, but some details may need another look.
| Integration packages with `requires` can depend on input or content packages by declaring them under | ||
| `requires` in `manifest.yml`. Depending on the package type, dependencies are resolved |
There was a problem hiding this comment.
Redundant requires?
| Integration packages with `requires` can depend on input or content packages by declaring them under | |
| `requires` in `manifest.yml`. Depending on the package type, dependencies are resolved | |
| Integration packages can depend on input or content packages by declaring them under | |
| `requires` in `manifest.yml`. Depending on the package type, dependencies are resolved |
There was a problem hiding this comment.
I've rephrase it. thnks
| ``` | ||
|
|
||
| ## Composable packages and the package registry | ||
| ## Integrations with requires and the package registry |
There was a problem hiding this comment.
Thanks 👍
Maybe instead of "requires" we can say "required packages", or "dependencies".
| ## Integrations with requires and the package registry | |
| ## Integrations with dependencies and the package registry |
| ## Integrations with requires and the package registry | |
| ## Integrations with required packages and the package registry |
Or backquote requires so it refers to the setting in the manifest.
| ## Integrations with requires and the package registry | |
| ## Integrations with `requires` and the package registry |
| integration: ">9.5.0,<9.6.0", | ||
| dependency: "^9.6.0", | ||
| want: false, | ||
| }, |
There was a problem hiding this comment.
Could we add a test case like this one? This would be a common case when integrations start supporting new majors.
| }, | |
| { | |
| name: "strict-greater range does not overlap higher constraint", | |
| integration: "^9.5.0 || ^10.0.0", | |
| dependency: "^9.6.0", | |
| want: false, | |
| }, |
| integration: ">=9.4.0,<9.6.0", | ||
| dependency: "^9.6.0", |
There was a problem hiding this comment.
A more realistic case that should be also a non-overlapping range, would be this one:
| integration: ">=9.4.0,<9.6.0", | |
| dependency: "^9.6.0", | |
| integration: "^9.4.0", | |
| dependency: "^9.6.0", |
| name: "empty integration constraint always overlaps", | ||
| integration: "", | ||
| dependency: "^9.6.0", | ||
| want: true, |
There was a problem hiding this comment.
This should fail. This means that the integration doesn't have any kibana restriction, but the input has. The resulting built integration should also have ^9.6.0, not an empty constraint.
| name: "empty integration constraint always overlaps", | |
| integration: "", | |
| dependency: "^9.6.0", | |
| want: true, | |
| name: "empty integration constraint always overlaps", | |
| integration: "", | |
| dependency: "^9.6.0", | |
| want: false, |
There was a problem hiding this comment.
Try to reuse internal/yamledit, it already has helpers for the kind of functionality needed here.
There was a problem hiding this comment.
Replaced hand-rolled YAML helpers with yamledit.NewDocumentBytes, GetSequenceNode, SetKeyValue, and Write.
| } | ||
|
|
||
| // Update resolves and optionally applies dependency version bumps for an integration package with requires. | ||
| func Update(opts Options) (*Result, error) { |
There was a problem hiding this comment.
Instead of having a single method that resolves and optionally applies version dumps, and given that we already have the type for proposals, maybe we should have two methods, one to resolve, and another one to apply.
func Resolve(opts Options) ([]UpdateProposal, error)
func Apply(manifest []bytes, proposals []UpdateProposal) ([]bytes, error)
There was a problem hiding this comment.
Done. Resolve handles resolution only; Apply rewrites manifest bytes in memory; the command layer owns the disk write. Dry-run is a natural consequence of skipping Apply.
| }, nil | ||
| } | ||
|
|
||
| func fetchCompatibleRevisions(opts Options, integrationKibana, packageName string) ([]packages.PackageManifest, error) { |
There was a problem hiding this comment.
This method gets the versions by querying from all the derived kibana versions. This makes several queries to the registry, not sure if this will be fully accurate for what we need.
Maybe an initial approach while we don't have another way to get packages from a given constraint from the registry is to query all versions of a package with a single request (with all=true), and then do the filtering locally.
There was a problem hiding this comment.
fetchAllRevisions now issues a single request with All: true and filters locally in filterByKibanaSubset.
| Prerelease: true, | ||
| Experimental: true, |
There was a problem hiding this comment.
Umm, we probably don't want to update to prerelease versions, at least by default. This can be another reason to just query all versions of the package and then filter locally. An exception could be a dependency that only has prerelease versions.
There could be a flag to decide if we want or not to update to prerelease versions, but getting them should not be the default.
There was a problem hiding this comment.
as all the otel input packages are still a prerelease, that is why i configured as so, i will check for the flag so we can use it instead
There was a problem hiding this comment.
Yeah, we can keep an exception for packages that only have prerelease versions. But for GA packages that use input packages that have GA versions we should definitely default to GA versions.
Btw, in principle requires can be used too with non-OTel packages, so we could create now an integration package that requires input packages with GA versions.
| return proposals, nil | ||
| } | ||
|
|
||
| func resolveDependency(opts Options, integrationKibana string, kind DependencyKind, dep packages.PackageDependency) (*UpdateProposal, error) { |
There was a problem hiding this comment.
Nit. We could have some unit tests for this using httptest and/or with go-vcr to record real interactions.
kibanaConstraintIsSubset verifies that every version satisfying the integration's kibana constraint also satisfies the dependency's, rather than merely requiring some version to satisfy both. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds the four test cases specified in the PR review (floor-below-dep, empty integration, OR-branch, OR-ordering invariant), fixes the empty-integration expected value from true→false, and renames filterByKibanaOverlap → filterByKibanaSubset to match the new semantics. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…rerelease flag Replace the per-Kibana-version loop in fetchCompatibleRevisions with a single registry call using all=true, then filter locally with filterByKibanaSubset. Add --prerelease flag (default off) to requires update; fall back to pre-releases only when a dependency has no stable versions available. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ply+write to cmd Remove DryRun from Options and Applied from Result. Resolve reads the manifest and returns proposals; callers decide whether to call Apply and write to disk. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replaces the bespoke gopkg.in/yaml.v3 byte-surgery implementation in manifest.go with calls to internal/yamledit (goccy/go-yaml AST-based), removing four helper functions and the yaml.v3 dependency from the file. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add -v debug output covering the full resolution pipeline: registry URL, package + kibana constraint, per-dependency fetch counts, per-revision kibana compatibility checks, prerelease fallback, and manifest writes. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Rename the section heading to "Integrations with required packages and the package registry" and remove the redundant "with `requires`" phrase from the opening sentence. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…equires update Adds NewJSONFormatter() to internal/formatter as a version-less encoder for content that is not part of the package spec. JSONFormatterBuilder retains its spec-version gate for spec documents (sample events, test results) and its doc now points non-spec callers to NewJSONFormatter. cmd/requires update switches from a hand-rolled json.NewEncoder to NewJSONFormatter, removing the encoding/json dependency from that file. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds TestFetchAllRevisions covering query parameter correctness, prerelease forwarding, stable-to-prerelease fallback logic, no-fallback when stable versions exist, server error propagation, and empty-response behaviour. Uses a requestCapturingServer helper (httptest + mutex) to inspect outgoing HTTP requests without real network calls. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Consolidates individual TestResolve_* functions into a single TestResolve table and TestApply_* functions into TestApply, following the same pattern already used in TestFetchAllRevisions. The manifest string is passed to each check function so the apply round-trip case can remain self-contained. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Standardizes the formatting of flag definitions in flags.go by aligning the spacing and indentation for improved readability. No functional changes were made.
jsoriano
left a comment
There was a problem hiding this comment.
LGTM, thanks for addressing the comments.
If you don't add the flag to add the changelog in this PR please create a follow up issue (or PR).
💚 Build Succeeded
History
|
i've opened a followup here #3626 |
mrodm
left a comment
There was a problem hiding this comment.
Tested locally and it looks great!
Thanks!
What
Adds
elastic-package requires update— a new subcommand that readsrequires.inputandrequires.contententries frommanifest.yml, queries the package registry for the latest published versions, and writes the updated pins back to the file.Key behaviours:
requiresblock; other package types are skipped.conditions.kibana.version: a newer dependency version is only proposed when its Kibana constraint overlaps with the integration's.conditions.kibana.version.--dry-runreports proposals without modifying the file.--format jsonemits machine-readable output suited for CI batch jobs.^0.3.0) to exact semver on write.Output example:
Why
Integration packages pin exact versions of input/content packages in
manifest.yml. Keeping those pins up to date today requires manually checking the registry and editing the file — error-prone at scale across the integrations repository.requires updatemakes bumping pins a single command, safe to run in CI across all integration packages, with--format jsonoutput for grouping PRs by code owner.Kibana compatibility check
To determine whether a newer dependency version is compatible, the command checks whether the integration's and dependency's
conditions.kibana.versionconstraints share any valid Kibana version.