fix: support upstream specs in component query - #110
fix: support upstream specs in component query#110Agyei Holy (holly-agyei) wants to merge 3 commits into
Conversation
|
reuben olinsky (@reubeno) This PR is ready for review. It adds support for upstream/default specs in |
There was a problem hiding this comment.
Pull request overview
Fixes azldev component query for upstream/default-upstream components by ensuring specs are made locally available before parsing, aligning behavior with other component subcommands.
Changes:
- Added a spec-preparation step in
component queryfor non-local spec sources before parsing. - Normalized
SpecSourceTypeUnspecifiedtoupstreamfor the query-only path. - Added a regression test covering the default/unspecified upstream component case.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/app/azldev/cmds/component/query.go | Adds upstream source preparation + normalization so query can parse specs from prepared local paths. |
| internal/app/azldev/cmds/component/query_test.go | Adds a regression test for querying a default upstream (unspecified) component. |
| preparer, err := sources.NewPreparer(sourceManager, env.FS(), env, env, sources.WithSkipLookaside()) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to create source preparer:\n%w", err) | ||
| } | ||
|
|
||
| workDirFactory, err := workdir.NewFactory(env.FS(), env.WorkDir(), env.ConstructionTime()) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to create work dir factory:\n%w", err) | ||
| } | ||
|
|
||
| preparedSourcesDir, err := workDirFactory.Create(comp.GetName(), "query-spec") | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to create work dir for component %#q:\n%w", comp.GetName(), err) | ||
| } | ||
|
|
||
| if err := preparer.PrepareSources(env, componentForPrep, preparedSourcesDir, true /* applyOverlays */); err != nil { | ||
| return nil, fmt.Errorf("failed to prepare sources for component %#q:\n%w", comp.GetName(), err) |
There was a problem hiding this comment.
parseComponentSpec prepares upstream sources without enabling dist-git / synthetic history (sources.WithGitRepo). In other paths (e.g., component render) WithGitRepo is used so rpmautospec can expand %autorelease / %autochangelog correctly and to keep release numbering consistent with overlay commit count. As written, component query may produce incorrect release/changelog values (or fail) for specs that rely on rpmautospec because the upstream .git dir is removed and no synthetic commits are created. Consider aligning this with render by adding sources.WithGitRepo(env.Config().Project.DefaultAuthorEmail) (or otherwise preserving .git + synthetic history) for this query-only prep path, or explicitly disabling rpmautospec-dependent expansion and documenting the limitation.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| component: projectconfig.ComponentConfig{ | ||
| Name: testComponentName, | ||
| Spec: projectconfig.SpecConfig{ | ||
| SourceType: projectconfig.SpecSourceTypeUpstream, | ||
| UpstreamName: testUpstreamName, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
projectconfig.SpecConfig does not exist (the ComponentConfig.Spec field is projectconfig.SpecSource). This won’t compile; use the same spec struct type as in TestQueryComponents_OneComponent when setting SourceType/UpstreamName.
There was a problem hiding this comment.
Copilot apply changes based on this feedback
| testEnv.CmdFactory.RunAndGetOutputHandler = func(cmd *exec.Cmd) (string, error) { | ||
| if len(cmd.Args) >= 5 && | ||
| cmd.Args[0] == "git" && | ||
| cmd.Args[1] == "-C" && | ||
| cmd.Args[3] == "rev-parse" && | ||
| cmd.Args[4] == "HEAD" { | ||
| return "head123abc\n", nil | ||
| } | ||
|
|
||
| return "name=test-component\nepoch=0\nversion=1.0.0\nrelease=1.azl3\n", nil | ||
| } |
There was a problem hiding this comment.
This test writes different Name: values into the generated .spec, but the mocked rpmspec output is hard-coded to name=test-component and the assertions don’t check the returned spec name/version fields. As written, the test won’t fail if the query path parses the wrong spec (or never reads it). Consider making the mock output depend on the test case and asserting on result.Name (and/or other parsed fields) to actually validate upstream vs default behavior.
There was a problem hiding this comment.
Copilot apply changes based on this feedback
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (3)
internal/app/azldev/cmds/component/query_test.go:109
projectconfig.ComponentConfig.Specis aprojectconfig.SpecSource, but this test usesprojectconfig.SpecConfig(which doesn’t exist). This won’t compile; setSpecusingprojectconfig.SpecSourcelike the other tests.
Spec: projectconfig.SpecConfig{
SourceType: projectconfig.SpecSourceTypeUpstream,
UpstreamName: testUpstreamName,
},
internal/app/azldev/cmds/component/query_test.go:149
- The mocked spec-query output is hard-coded to
name=test-component, so the two test cases can’t distinguish between default/normalized upstream behavior vs explicitUpstreamName. Make the mocked output depend on the test case so the assertions can catch regressions.
return "name=test-component\nepoch=0\nversion=1.0.0\nrelease=1.azl3\n", nil
}
internal/app/azldev/cmds/component/query_test.go:162
- This assertion only checks
result.Name == testComponentName, which will pass even when the upstream spec name differs (and even if the query path accidentally always uses the normalized name). Assert against the per-test expected name so the test actually validates the behavior under test.
result := results[0]
assert.Equal(t, testComponentName, result.Name)
Nan Liu (liunan-ms)
left a comment
Nan Liu (liunan-ms)
left a comment
There was a problem hiding this comment.
Agyei Holy (@holly-agyei) Thanks for contributing to azure-linux-dev-tools! I left some comments to improve the fix.
| preparedSourcesDir, err := workDirFactory.Create(comp.GetName(), "query-spec") | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to create work dir for component %#q:\n%w", comp.GetName(), err) | ||
| } |
There was a problem hiding this comment.
[blocking]: The prepared work dir is leaked. workDirFactory.Create returns a real directory under WorkDir() and doesn't self-clean, and this function never removes preparedSourcesDir. Since query runs this per component in the loop, every invocation leaves one clone/prepared tree per non-local component behind, growing without bound. render.go pairs its staging dir with a deferred cleanup and the same cleanup can be done here:
preparedSourcesDir, err := workDirFactory.Create(comp.GetName(), "query-spec")
if err != nil {
return nil, fmt.Errorf("failed to create work dir for component %#q:\n%w", comp.GetName(), err)
}
defer func() {
if rmErr := env.FS().RemoveAll(preparedSourcesDir); rmErr != nil {
slog.Debug("failed to remove query work dir", "path", preparedSourcesDir, "error", rmErr)
}
}()
|
|
||
| preparedConfig := *componentForPrep.GetConfig() | ||
| preparedConfig.Spec.SourceType = projectconfig.SpecSourceTypeLocal | ||
| preparedConfig.Spec.Path = filepath.Join(preparedSourcesDir, comp.GetName()+".spec") |
There was a problem hiding this comment.
The hardcoded <name>.spec join is correct here, but render.go already has a findSpecFile helper that does this join plus an existence check. Reusing it would keep the two paths consistent and give a clearer error if the spec is missing after preparation.
| results, err := component.QueryComponents(testEnv.Env, &options) | ||
| require.NoError(t, err) | ||
| require.Len(t, results, 1) | ||
|
|
There was a problem hiding this comment.
Given the work-dir leak noted in query.go, please add an assertion that the prepared sources dir is cleaned up after QueryComponents returns (e.g. assert WorkDir() contains no leftover query-spec tree). That locks in the cleanup fix and prevents a future regression. An error-path case (PrepareSources failing) would be a nice-to-have too.
Fixes #19
Summary
component queryTest plan
GOCACHE=/tmp/azldev-gocache GOMODCACHE=/tmp/azldev-gomodcache go test ./internal/app/azldev/cmds/component -run 'Test(NewComponentQueryCommand|ComponentQueryCmd_NoMatch|QueryComponents_.*)$' -count=1