diff --git a/docs/user/reference/cli/azldev_component_build.md b/docs/user/reference/cli/azldev_component_build.md index 25390853..2086f683 100644 --- a/docs/user/reference/cli/azldev_component_build.md +++ b/docs/user/reference/cli/azldev_component_build.md @@ -57,7 +57,7 @@ azldev component build [flags] --mock-config-opt stringToString Pass a configuration option through to mock (key=value, can be specified multiple times) (default []) --no-check Skip package %check tests --preserve-buildenv policy Preserve build environment {on-failure, always, never} (default on-failure) - --skip-lock-validation skip lock file consistency checks (default true) + --skip-lock-validation skip lock file consistency checks -s, --spec-path stringArray Spec path --srpm-only Build SRPM (source RPM) *only* --without-git Skip creating a dist-git repository with synthetic commit history diff --git a/docs/user/reference/cli/azldev_component_diff-sources.md b/docs/user/reference/cli/azldev_component_diff-sources.md index 8884d91c..afd471ac 100644 --- a/docs/user/reference/cli/azldev_component_diff-sources.md +++ b/docs/user/reference/cli/azldev_component_diff-sources.md @@ -22,7 +22,7 @@ azldev component diff-sources [flags] -g, --component-group stringArray Component group name -h, --help help for diff-sources --output-file string write the diff output to a file instead of stdout - --skip-lock-validation skip lock file consistency checks (default true) + --skip-lock-validation skip lock file consistency checks -s, --spec-path stringArray Spec path ``` diff --git a/docs/user/reference/cli/azldev_component_list.md b/docs/user/reference/cli/azldev_component_list.md index 3c908a2a..2900def0 100644 --- a/docs/user/reference/cli/azldev_component_list.md +++ b/docs/user/reference/cli/azldev_component_list.md @@ -38,7 +38,6 @@ azldev component list [flags] -p, --component stringArray Component name pattern -g, --component-group stringArray Component group name -h, --help help for list - --skip-lock-validation skip lock file consistency checks (default true) -s, --spec-path stringArray Spec path ``` diff --git a/docs/user/reference/cli/azldev_component_prepare-sources.md b/docs/user/reference/cli/azldev_component_prepare-sources.md index f605ac64..9e22b5c5 100644 --- a/docs/user/reference/cli/azldev_component_prepare-sources.md +++ b/docs/user/reference/cli/azldev_component_prepare-sources.md @@ -39,7 +39,7 @@ azldev component prepare-sources [flags] --force delete and recreate the output directory if it already exists -h, --help help for prepare-sources -o, --output-dir string output directory - --skip-lock-validation skip lock file consistency checks (default true) + --skip-lock-validation skip lock file consistency checks --skip-overlays skip applying overlays to prepared sources -s, --spec-path stringArray Spec path --without-git Disable dist-git repository creation (enabled by default) diff --git a/docs/user/reference/cli/azldev_component_query.md b/docs/user/reference/cli/azldev_component_query.md index c1604a9e..03a49fe1 100644 --- a/docs/user/reference/cli/azldev_component_query.md +++ b/docs/user/reference/cli/azldev_component_query.md @@ -34,7 +34,7 @@ azldev component query [flags] -p, --component stringArray Component name pattern -g, --component-group stringArray Component group name -h, --help help for query - --skip-lock-validation skip lock file consistency checks (default true) + --skip-lock-validation skip lock file consistency checks -s, --spec-path stringArray Spec path ``` diff --git a/docs/user/reference/cli/azldev_component_render.md b/docs/user/reference/cli/azldev_component_render.md index 5f483f6a..1ded12de 100644 --- a/docs/user/reference/cli/azldev_component_render.md +++ b/docs/user/reference/cli/azldev_component_render.md @@ -62,7 +62,7 @@ azldev component render [flags] -f, --force allow overwriting existing rendered component directories -h, --help help for render -o, --output-dir string output directory for rendered specs (overrides rendered-specs-dir from config) - --skip-lock-validation skip lock file consistency checks (default true) + --skip-lock-validation skip lock file consistency checks -s, --spec-path stringArray Spec path ``` diff --git a/docs/user/reference/cli/azldev_component_update.md b/docs/user/reference/cli/azldev_component_update.md index 49f84b70..91144579 100644 --- a/docs/user/reference/cli/azldev_component_update.md +++ b/docs/user/reference/cli/azldev_component_update.md @@ -61,7 +61,6 @@ azldev component update [flags] -g, --component-group stringArray Component group name --force-recalculate force re-resolution of all components, ignoring freshness checks that would skip unchanged components. Use when upstream state may have changed independently of the snapshot time and the new commit is preferred -h, --help help for update - --skip-lock-validation skip lock file consistency checks (default true) -s, --spec-path stringArray Spec path ``` diff --git a/internal/app/azldev/cmds/component/changed.go b/internal/app/azldev/cmds/component/changed.go index e2792844..63a1b896 100644 --- a/internal/app/azldev/cmds/component/changed.go +++ b/internal/app/azldev/cmds/component/changed.go @@ -76,10 +76,6 @@ detected via lock file presence in the compared refs when using -a.`, RunE: azldev.RunFuncWithExtraArgs(func(env *azldev.Env, args []string) (interface{}, error) { options.ComponentFilter.ComponentNamePatterns = append(args, options.ComponentFilter.ComponentNamePatterns...) - // Skip lock validation -- this command inspects historical locks at - // arbitrary refs, so HEAD-state validation is irrelevant. - options.ComponentFilter.SkipLockValidation = true - return ChangedComponents(env, options) }), ValidArgsFunction: components.GenerateComponentNameCompletions, @@ -123,6 +119,10 @@ const ( func ChangedComponents( env *azldev.Env, options *ChangedComponentOptions, ) ([]ChangedResult, error) { + // Changed compares lock files between git refs — skip validation since + // the current working-tree locks may legitimately be stale. + options.ComponentFilter.SkipLockValidation = true + resolver := components.NewResolver(env) comps, err := resolver.FindComponents(&options.ComponentFilter) diff --git a/internal/app/azldev/cmds/component/list.go b/internal/app/azldev/cmds/component/list.go index 3671b16d..ac348c81 100644 --- a/internal/app/azldev/cmds/component/list.go +++ b/internal/app/azldev/cmds/component/list.go @@ -48,10 +48,6 @@ Component name patterns support glob syntax (*, ?, []).`, RunE: azldev.RunFuncWithExtraArgs(func(env *azldev.Env, args []string) (interface{}, error) { options.ComponentFilter.ComponentNamePatterns = append(args, options.ComponentFilter.ComponentNamePatterns...) - // List is read-only — skip lock validation so users can always - // inspect their components even when locks are stale. - options.ComponentFilter.SkipLockValidation = true - return ListComponentConfigs(env, options) }), ValidArgsFunction: components.GenerateComponentNameCompletions, @@ -61,15 +57,25 @@ Component name patterns support glob syntax (*, ?, []).`, components.AddComponentFilterOptionsToCommand(cmd, &options.ComponentFilter) + // List always skips lock validation (read-only), so the flag is + // meaningless here. Hide it to avoid confusion. + _ = cmd.Flags().MarkHidden("skip-lock-validation") + return cmd } -// Lists components in the env, in accordance with options. Returns the found components. +// ListComponentConfigs lists components in the env, in accordance with options. +// Lock validation is always skipped regardless of the caller's SkipLockValidation +// value — list is read-only. func ListComponentConfigs( env *azldev.Env, options *ListComponentOptions, ) (results []projectconfig.ComponentConfig, err error) { var comps *components.ComponentSet + // List is read-only — skip lock validation so it works even when + // locks are missing or stale. + options.ComponentFilter.SkipLockValidation = true + resolver := components.NewResolver(env) comps, err = resolver.FindComponents(&options.ComponentFilter) diff --git a/internal/app/azldev/cmds/component/update.go b/internal/app/azldev/cmds/component/update.go index 7ff63e00..906162fa 100644 --- a/internal/app/azldev/cmds/component/update.go +++ b/internal/app/azldev/cmds/component/update.go @@ -87,9 +87,6 @@ Cannot be combined with --bump.`, # CI gate: exit 0 if locks are fresh, 1 if anything would change azldev component update -a --check-only -q`, RunE: azldev.RunFuncWithExtraArgs(func(env *azldev.Env, args []string) (interface{}, error) { - // Skip lock validation -- update is the lock file writer. - options.ComponentFilter.SkipLockValidation = true - options.ComponentFilter.ComponentNamePatterns = append( args, options.ComponentFilter.ComponentNamePatterns..., ) @@ -116,6 +113,10 @@ Cannot be combined with --bump.`, cmd.MarkFlagsMutuallyExclusive("bump", "check-only") + // Update always skips lock validation (it's the lock writer), so the + // flag is meaningless here. Hide it to avoid confusion. + _ = cmd.Flags().MarkHidden("skip-lock-validation") + return cmd } @@ -149,6 +150,8 @@ type UpdateResult struct { // UpdateComponents resolves source identities for all selected components and // writes the results to per-component lock files under locks/. +// Lock validation is always skipped regardless of the caller's SkipLockValidation +// value — update is the lock writer. func UpdateComponents(env *azldev.Env, options *UpdateComponentOptions) ([]UpdateResult, error) { if options.Bump && options.CheckOnly { return nil, fmt.Errorf("%w: --bump and --check-only are mutually exclusive", azldev.ErrInvalidUsage) @@ -158,6 +161,9 @@ func UpdateComponents(env *azldev.Env, options *UpdateComponentOptions) ([]Updat // Suppress staleness warnings — we're about to refresh the locks ourselves, // so warning the user to "run component update" would be self-referential noise. resolver.SuppressLockWarnings = true + // Skip lock validation — update is the lock file writer, so missing or + // stale locks are expected and will be fixed by this command. + options.ComponentFilter.SkipLockValidation = true // Enable freshness checking so the resolver computes FreshnessStatus for // each component. This lets resolveSourceIdentitiesParallel skip // re-resolution for components whose resolution inputs haven't changed. diff --git a/internal/app/azldev/cmds/component/update_test.go b/internal/app/azldev/cmds/component/update_test.go index 0b196271..57dea644 100644 --- a/internal/app/azldev/cmds/component/update_test.go +++ b/internal/app/azldev/cmds/component/update_test.go @@ -99,7 +99,9 @@ func setupMockGit(env *testutils.TestEnv, commitHash string) { } } -// addUpstreamComponent registers an upstream component in the test config. +// addUpstreamComponent adds an upstream component to the test config +// without writing a lock file. Used by update tests where the update command +// itself creates the lock. func addUpstreamComponent(env *testutils.TestEnv, name string) { env.Config.Components[name] = projectconfig.ComponentConfig{ Name: name, diff --git a/internal/app/azldev/core/components/filter.go b/internal/app/azldev/core/components/filter.go index 678620cb..95c9b582 100644 --- a/internal/app/azldev/core/components/filter.go +++ b/internal/app/azldev/core/components/filter.go @@ -4,7 +4,6 @@ package components import ( - "os" "strings" "github.com/microsoft/azure-linux-dev-tools/internal/app/azldev" @@ -25,10 +24,7 @@ type ComponentFilter struct { IncludeAllComponents bool // SkipLockValidation disables lock file consistency checks for this // filter's resolution. Commands that write lock files (update) or are - // read-only (list) set this to true. The '--skip-lock-validation' flag - // defaults based on AZLDEV_ENABLE_LOCK_VALIDATION during rollout. - //nolint:godox // tracked by TODO(lockfiles) tag. - // TODO(lockfiles): remove env var gate; default to false (validation on). + // read-only (list) set this to true. SkipLockValidation bool } @@ -58,7 +54,7 @@ func AddComponentFilterOptionsToCommand(cmd *cobra.Command, filter *ComponentFil _ = cmd.MarkFlagFilename("spec-path", ".spec") cmd.Flags().BoolVar(&filter.SkipLockValidation, "skip-lock-validation", - os.Getenv("AZLDEV_ENABLE_LOCK_VALIDATION") != "1", + false, "skip lock file consistency checks") } diff --git a/internal/app/azldev/core/components/resolver.go b/internal/app/azldev/core/components/resolver.go index 1ff6e843..5001ac1b 100644 --- a/internal/app/azldev/core/components/resolver.go +++ b/internal/app/azldev/core/components/resolver.go @@ -7,7 +7,6 @@ import ( "errors" "fmt" "log/slog" - "os" "path" "path/filepath" "strings" @@ -54,13 +53,10 @@ func NewResolver(env *azldev.Env) *Resolver { // Given a component filter, finds all components defined in the environment that match the filter. func (r *Resolver) FindComponents(filter *ComponentFilter) (components *ComponentSet, err error) { // The filter's SkipLockValidation field is the primary control. When - // created via AddComponentFilterOptionsToCommand, its default comes from - // the AZLDEV_ENABLE_LOCK_VALIDATION env var. For programmatic callers - // (including tests), also check the env var here so the rollout gate - // applies uniformly. - //nolint:godox // tracked by TODO(lockfiles) tag. - // TODO(lockfiles): remove env var gate; filter.SkipLockValidation becomes sole control. - skipValidation := filter.SkipLockValidation || os.Getenv("AZLDEV_ENABLE_LOCK_VALIDATION") != "1" + // created via AddComponentFilterOptionsToCommand, its default is false + // (validation on). Commands that write locks (update) or are read-only + // (list) explicitly set it to true. + skipValidation := filter.SkipLockValidation // For usability's sake, detect if the caller/user forgot to specify *any* criteria. if filter.HasNoCriteria() { diff --git a/internal/app/azldev/core/components/resolver_test.go b/internal/app/azldev/core/components/resolver_test.go index 5dc2e516..3b09efc9 100644 --- a/internal/app/azldev/core/components/resolver_test.go +++ b/internal/app/azldev/core/components/resolver_test.go @@ -56,6 +56,9 @@ func addTestComponentToConfig(t *testing.T, env *testutils.TestEnv) projectconfi env.Config.Components[component.Name] = component + // Write a minimal lock so lock validation passes. + env.WriteDefaultLock(t, component.Name) + return component } @@ -74,12 +77,17 @@ func TestFindComponents_AllComponents(t *testing.T) { filter := &components.ComponentFilter{IncludeAllComponents: true} // Find! - components, err := components.NewResolver(env.Env).FindComponents(filter) + resolved, err := components.NewResolver(env.Env).FindComponents(filter) require.NoError(t, err) - require.Len(t, components.Components(), 1) + require.Len(t, resolved.Components(), 1) - actualComponent := components.Components()[0] - require.Equal(t, &expectedComponent, actualComponent.GetConfig()) + actualComponent := resolved.Components()[0] + + // Locked is populated at resolve time from the lock file — clear it + // so we can compare just the config fields set by the test. + actualConfig := *actualComponent.GetConfig() + actualConfig.Locked = nil + require.Equal(t, expectedComponent, actualConfig) } func TestFindComponents_NonExistentSpecPaths(t *testing.T) { @@ -169,7 +177,12 @@ func TestFindComponents_MatchingNamedPattern(t *testing.T) { require.Len(t, components.Components(), 1) actualComponent := components.Components()[0] - assert.Equal(t, &component, actualComponent.GetConfig()) + + // Locked is populated at resolve time from the lock file — clear it + // so we can compare just the config fields set by the test. + actualConfig := *actualComponent.GetConfig() + actualConfig.Locked = nil + assert.Equal(t, component, actualConfig) } func TestFindComponents_FoundGroups(t *testing.T) { @@ -225,7 +238,12 @@ func TestFindAllComponents_SomeComponents(t *testing.T) { require.Len(t, components.Components(), 1) actualComponent := components.Components()[0] - assert.Equal(t, &expectedComponent, actualComponent.GetConfig()) + + // Locked is populated at resolve time from the lock file — clear it + // so we can compare just the config fields set by the test. + actualConfig := *actualComponent.GetConfig() + actualConfig.Locked = nil + assert.Equal(t, expectedComponent, actualConfig) } func TestFindAllComponents_MergesComponentPresentBySpecAndConfig(t *testing.T) { @@ -438,6 +456,8 @@ func TestApplyInheritedDefaults_GroupDefaults(t *testing.T) { } env.Config.Components[component.Name] = component + env.WriteDefaultLock(t, component.Name) + // Set up a group with default build config. env.Config.ComponentGroups["my-group"] = projectconfig.ComponentGroupConfig{ Components: []string{"my-comp"}, @@ -472,6 +492,8 @@ func TestApplyInheritedDefaults_MultipleGroupsDeterministicOrder(t *testing.T) { component := projectconfig.ComponentConfig{Name: "my-comp"} env.Config.Components[component.Name] = component + env.WriteDefaultLock(t, component.Name) + // Group "aaa" adds with=["from-aaa"]. env.Config.ComponentGroups["aaa"] = projectconfig.ComponentGroupConfig{ Components: []string{"my-comp"}, @@ -520,6 +542,8 @@ func TestApplyInheritedDefaults_ComponentOverridesGroupDefaults(t *testing.T) { } env.Config.Components[component.Name] = component + env.WriteDefaultLock(t, component.Name) + env.Config.ComponentGroups["my-group"] = projectconfig.ComponentGroupConfig{ Components: []string{"my-comp"}, DefaultComponentConfig: projectconfig.ComponentConfig{ @@ -557,6 +581,8 @@ func TestApplyInheritedDefaults_NoGroupMembership(t *testing.T) { } env.Config.Components[component.Name] = component + env.WriteDefaultLock(t, component.Name) + filter := &components.ComponentFilter{IncludeAllComponents: true} result, err := components.NewResolver(env.Env).FindComponents(filter) @@ -591,6 +617,8 @@ func TestApplyInheritedDefaults_ProjectDefault(t *testing.T) { } env.Config.Components[component.Name] = component + env.WriteDefaultLock(t, component.Name) + filter := &components.ComponentFilter{IncludeAllComponents: true} result, err := components.NewResolver(env.Env).FindComponents(filter) @@ -635,6 +663,8 @@ func TestApplyInheritedDefaults_ProjectDefaultOverriddenByGroup(t *testing.T) { component := projectconfig.ComponentConfig{Name: "my-comp"} env.Config.Components[component.Name] = component + env.WriteDefaultLock(t, component.Name) + filter := &components.ComponentFilter{IncludeAllComponents: true} result, err := components.NewResolver(env.Env).FindComponents(filter) @@ -719,19 +749,12 @@ func TestFindComponents_PopulatesLockedData(t *testing.T) { "Spec.UpstreamCommit should remain empty — lock data goes into Locked, not Spec") } -// When no lock file exists (e.g., new component that hasn't been updated yet), -// the Locked field should be nil. Downstream commands will fall back to the old -// resolution path (snapshot/HEAD) until the user runs 'component update'. -// -// TODO(lockfiles): Once lock validation is default-on, a missing lock for an -// upstream component should be an error, not a silent nil. Update this test to -// expect FindComponents to return an error, and remove the fallback path. -// -//nolint:godox // tracked by TODO(lockfiles) tag. -func TestFindComponents_LockedNilWhenNoLockFile(t *testing.T) { +// When no lock file exists for an upstream component, FindComponents should +// return a validation error telling the user to run 'component update' first. +func TestFindComponents_ErrorWhenNoLockFile(t *testing.T) { env := testutils.NewTestEnv(t) - // Add component with no lock file. + // Add upstream component with no lock file. env.Config.Components["bash"] = projectconfig.ComponentConfig{ Name: "bash", Spec: projectconfig.SpecSource{ @@ -741,6 +764,29 @@ func TestFindComponents_LockedNilWhenNoLockFile(t *testing.T) { filter := &components.ComponentFilter{IncludeAllComponents: true} + _, err := components.NewResolver(env.Env).FindComponents(filter) + require.Error(t, err) + assert.Contains(t, err.Error(), "lock file") +} + +// When SkipLockValidation is set, a missing lock file is tolerated — +// the component's Locked field is nil instead of causing an error. +func TestFindComponents_LockedNilWhenValidationSkipped(t *testing.T) { + env := testutils.NewTestEnv(t) + + // Add upstream component with no lock file. + env.Config.Components["bash"] = projectconfig.ComponentConfig{ + Name: "bash", + Spec: projectconfig.SpecSource{ + SourceType: projectconfig.SpecSourceTypeUpstream, + }, + } + + filter := &components.ComponentFilter{ + IncludeAllComponents: true, + SkipLockValidation: true, + } + resolved, err := components.NewResolver(env.Env).FindComponents(filter) require.NoError(t, err) @@ -772,7 +818,10 @@ func TestFindComponents_ExplicitPinPreserved(t *testing.T) { env.WriteLock(t, "curl", lock) - filter := &components.ComponentFilter{IncludeAllComponents: true} + filter := &components.ComponentFilter{ + IncludeAllComponents: true, + SkipLockValidation: true, + } resolved, err := components.NewResolver(env.Env).FindComponents(filter) require.NoError(t, err) @@ -870,7 +919,10 @@ func TestFindComponents_CorruptLockSurfaces(t *testing.T) { require.NoError(t, fileutils.WriteFile(env.TestFS, lockPath, []byte("this is not valid TOML \x00\x01\x02"), fileperms.PrivateFile)) - filter := &components.ComponentFilter{IncludeAllComponents: true} + filter := &components.ComponentFilter{ + IncludeAllComponents: true, + SkipLockValidation: true, + } resolved, err := components.NewResolver(env.Env).FindComponents(filter) require.NoError(t, err, "corrupt lock should warn, not fail (validation is separate)") @@ -901,7 +953,10 @@ func TestFindComponents_PinDiffersFromLock(t *testing.T) { env.WriteLock(t, "curl", lock) - filter := &components.ComponentFilter{IncludeAllComponents: true} + filter := &components.ComponentFilter{ + IncludeAllComponents: true, + SkipLockValidation: true, + } resolved, err := components.NewResolver(env.Env).FindComponents(filter) require.NoError(t, err) diff --git a/internal/app/azldev/core/sources/synthistory.go b/internal/app/azldev/core/sources/synthistory.go index bf9c4a27..9d66f152 100644 --- a/internal/app/azldev/core/sources/synthistory.go +++ b/internal/app/azldev/core/sources/synthistory.go @@ -8,7 +8,6 @@ import ( "errors" "fmt" "log/slog" - "os" "path/filepath" "slices" "strings" @@ -636,18 +635,14 @@ func readLockFileAtHEAD( lockFileRelPath, lockFileErr) } - // File genuinely missing — the component has no overlays, so the - // dist-git is just the upstream as-is and no synthetic commits - // are needed. - //nolint:godox // tracked by TODO(lockfiles) tag. - // TODO(lockfiles): remove env var gate and make this a hard error unconditionally. - if os.Getenv("AZLDEV_ENABLE_LOCK_VALIDATION") == "1" { - return nil, fmt.Errorf("lock file %#q not found at HEAD:\n%w", - lockFileRelPath, lockFileErr) - } - + // File genuinely missing — no committed lock at HEAD. This is normal + // for local components (no upstream commit) and for upstream components + // whose lock was created on disk but not yet committed to git. + // Note: the resolver validates lock existence on the *filesystem* (working + // tree), not in git — so a lock written by 'component update' but not + // yet committed passes resolver validation but is absent at HEAD. slog.Debug("No lock file found at HEAD; skipping synthetic history", - "lockFile", lockFileRelPath, "error", lockFileErr) + "lockFile", lockFileRelPath, "reason", lockFileErr) return nil, nil //nolint:nilnil // nil,nil signals "not found, skip" to caller. } diff --git a/internal/app/azldev/core/testutils/testenv.go b/internal/app/azldev/core/testutils/testenv.go index 38df2959..c9d4e4f0 100644 --- a/internal/app/azldev/core/testutils/testenv.go +++ b/internal/app/azldev/core/testutils/testenv.go @@ -197,3 +197,35 @@ func (e *TestEnv) WriteLock(t *testing.T, name string, lock *lockfile.ComponentL store := lockfile.NewStore(e.TestFS, "/project/"+projectconfig.DefaultLockDir) require.NoError(t, store.Save(name, lock)) } + +// TestUpstreamCommit is the default commit hash used by [TestEnv.WriteDefaultLock] +// and [TestEnv.AddUpstreamComponent]. +const TestUpstreamCommit = "test-upstream-commit-aabb1122" + +// WriteDefaultLock writes a minimal valid lock file for the named component. +// Use this when a test adds a component inline (with custom config) and just +// needs lock validation to pass. +func (e *TestEnv) WriteDefaultLock(t *testing.T, name string) { + t.Helper() + + lock := lockfile.New() + lock.UpstreamCommit = TestUpstreamCommit + e.WriteLock(t, name, lock) +} + +// AddUpstreamComponent registers an upstream component in the test config and +// writes a minimal lock file so lock validation passes. Use this instead of +// setting env.Config.Components directly when the test doesn't need to +// customize the lock data. +func (e *TestEnv) AddUpstreamComponent(t *testing.T, name string) { + t.Helper() + + e.Config.Components[name] = projectconfig.ComponentConfig{ + Name: name, + Spec: projectconfig.SpecSource{ + SourceType: projectconfig.SpecSourceTypeUpstream, + }, + } + + e.WriteDefaultLock(t, name) +} diff --git a/internal/projectconfig/component.go b/internal/projectconfig/component.go index 9f0593ad..7c6b1c89 100644 --- a/internal/projectconfig/component.go +++ b/internal/projectconfig/component.go @@ -297,12 +297,8 @@ func (c *ComponentConfig) MergeUpdatesFrom(other *ComponentConfig) error { // EffectiveUpstreamCommit returns the commit to use for upstream operations. // Prefers the locked commit (resolved reality) over the config pin (user intent). -// Returns empty string when neither is set. -// -// TODO(lockfiles): Once lock validation is default-on, drop the Spec.UpstreamCommit -// fallback - a missing lock will be a hard error before we reach this code. -// -//nolint:godox // tracked by TODO(lockfiles) tag. +// Falls back to Spec.UpstreamCommit for SkipLockValidation paths (update, list, +// changed) where Locked may be nil. Returns empty string when neither is set. func (c *ComponentConfig) EffectiveUpstreamCommit() string { if c.Locked != nil && c.Locked.UpstreamCommit != "" { return c.Locked.UpstreamCommit diff --git a/internal/projectconfig/configfile.go b/internal/projectconfig/configfile.go index 6eecd068..3dcf2551 100644 --- a/internal/projectconfig/configfile.go +++ b/internal/projectconfig/configfile.go @@ -6,7 +6,6 @@ package projectconfig import ( "fmt" "net/url" - "os" "github.com/go-playground/validator/v10" "github.com/microsoft/azure-linux-dev-tools/internal/global/opctx" @@ -93,20 +92,11 @@ func (f ConfigFile) Validate() error { } } - // Per-component snapshot timestamps are not allowed when lock validation is - // enabled. Components inherit the snapshot from the distro/group - // default-component-config or the project's default-distro. Per-component - // snapshots would create non-deterministic builds that the lock file cannot - // reliably track. Use an explicit 'upstream-commit' pin instead. - // - // Gated behind AZLDEV_ENABLE_LOCK_VALIDATION during rollout. - // NOTE: Once the env var gate is removed, snapshot rejection becomes - // unconditional — per-component snapshots are permanently disallowed. - // Do NOT move this into [ComponentFilter.SkipLockValidation]: the snapshot check - // must not be disableable post-rollout. - //nolint:godox // tracked by TODO(lockfiles) tag. - // TODO(lockfiles): remove env var gate; snapshot rejection becomes unconditional. - lockValidationEnabled := os.Getenv("AZLDEV_ENABLE_LOCK_VALIDATION") == "1" + // Per-component snapshot timestamps are not allowed. Components inherit + // the snapshot from the distro/group default-component-config or the + // project's default-distro. Per-component snapshots would create + // non-deterministic builds that the lock file cannot reliably track. + // Use an explicit 'upstream-commit' pin instead. // Validate overlay configurations for each component. for componentName, component := range f.Components { @@ -127,7 +117,7 @@ func (f ConfigFile) Validate() error { return err } - if lockValidationEnabled && component.Spec.UpstreamDistro.Snapshot != "" { + if component.Spec.UpstreamDistro.Snapshot != "" { return fmt.Errorf( "component %#q has a per-component 'snapshot' on 'upstream-distro'; "+ "snapshots should be set on the distro or group default-component-config, "+ diff --git a/internal/projectconfig/configfile_test.go b/internal/projectconfig/configfile_test.go index d7264d55..e8c7809d 100644 --- a/internal/projectconfig/configfile_test.go +++ b/internal/projectconfig/configfile_test.go @@ -301,8 +301,6 @@ func TestProjectConfigFileValidation_UnsupportedOriginType(t *testing.T) { } func TestProjectConfigFileValidation_PerComponentSnapshotDisallowed(t *testing.T) { - t.Setenv("AZLDEV_ENABLE_LOCK_VALIDATION", "1") - file := projectconfig.ConfigFile{ Components: map[string]projectconfig.ComponentConfig{ "test-component": { diff --git a/internal/providers/sourceproviders/fedorasourceprovider.go b/internal/providers/sourceproviders/fedorasourceprovider.go index 1d7212d7..344e90a9 100644 --- a/internal/providers/sourceproviders/fedorasourceprovider.go +++ b/internal/providers/sourceproviders/fedorasourceprovider.go @@ -249,8 +249,13 @@ func (g *FedoraSourcesProviderImpl) checkoutTargetCommit( } // ResolveIdentity implements [SourceIdentityProvider] by resolving the upstream -// commit hash for the component. Prefers the locked commit when available; -// falls back to live resolution (clone + snapshot/HEAD) when no lock exists. +// commit hash for the component. Ignores [projectconfig.ComponentLockData] — +// callers that want the cached locked commit should read +// [projectconfig.ComponentLockData.UpstreamCommit] directly. +// +// When [projectconfig.SpecSource.UpstreamCommit] is pinned, returns that commit +// directly without contacting upstream. Otherwise, clones the dist-git repo to +// resolve the commit via snapshot time or branch HEAD. func (g *FedoraSourcesProviderImpl) ResolveIdentity( ctx context.Context, component components.Component, @@ -259,14 +264,6 @@ func (g *FedoraSourcesProviderImpl) ResolveIdentity( return "", errors.New("component name cannot be empty") } - // Use locked commit if available — no clone needed. - //nolint:godox // tracked by TODO(lockfiles) tag. - // TODO(lockfiles): Once lock validation is default-on, a missing lock is an - // error caught before we get here. Remove the fallback to resolveCommit. - if locked := component.GetConfig().Locked; locked != nil && locked.UpstreamCommit != "" { - return locked.UpstreamCommit, nil - } - upstreamName := component.GetConfig().Spec.UpstreamName if upstreamName == "" { upstreamName = component.GetName() diff --git a/internal/providers/sourceproviders/fedorasourceprovider_test.go b/internal/providers/sourceproviders/fedorasourceprovider_test.go index 7c6909df..0d45e8fe 100644 --- a/internal/providers/sourceproviders/fedorasourceprovider_test.go +++ b/internal/providers/sourceproviders/fedorasourceprovider_test.go @@ -1044,8 +1044,8 @@ func TestCheckoutTargetCommit_UpstreamCommit(t *testing.T) { } // TestGetComponent_LockedCommitTakesPriorityOverConfigPin verifies that the -// fetch path (used by render/build) honors the same locked-beats-pin -// precedence rule as ResolveIdentity. When both Locked.UpstreamCommit and +// fetch path (used by render/build) uses EffectiveUpstreamCommit, which prefers +// the locked commit over the config pin. When both Locked.UpstreamCommit and // Spec.UpstreamCommit are set, Checkout must be called with the locked value. func TestGetComponent_LockedCommitTakesPriorityOverConfigPin(t *testing.T) { env := testutils.NewTestEnv(t) diff --git a/internal/providers/sourceproviders/identityprovider_test.go b/internal/providers/sourceproviders/identityprovider_test.go index 1e0246f2..8682090f 100644 --- a/internal/providers/sourceproviders/identityprovider_test.go +++ b/internal/providers/sourceproviders/identityprovider_test.go @@ -297,11 +297,13 @@ func (d *noOpDownloader) ExtractSourcesFromRepo( return nil } -// --- Lock data integration tests --- +// --- ResolveIdentity always resolves from upstream --- -// TestFedoraProvider_ResolveIdentity_UsesLockedCommit verifies that ResolveIdentity -// returns the locked commit immediately without cloning when Locked data is present. -func TestFedoraProvider_ResolveIdentity_UsesLockedCommit(t *testing.T) { +// TestFedoraProvider_ResolveIdentity_IgnoresLockedData verifies that +// ResolveIdentity does not short-circuit on locked data — it always +// resolves from upstream. Callers that want the cached locked commit should +// read ComponentLockData.UpstreamCommit directly. +func TestFedoraProvider_ResolveIdentity_IgnoresLockedData(t *testing.T) { ctrl := gomock.NewController(t) mockGitProvider := git_test.NewMockGitProvider(ctrl) @@ -315,51 +317,20 @@ func TestFedoraProvider_ResolveIdentity_UsesLockedCommit(t *testing.T) { ) require.NoError(t, err) - lockedCommit := "locked-abc123" + headCommit := "fresh-upstream-commit" comp := newMockCompWithConfig(ctrl, testPackageName, &projectconfig.ComponentConfig{ Name: testPackageName, Spec: projectconfig.SpecSource{ SourceType: projectconfig.SpecSourceTypeUpstream, + // No UpstreamCommit pin → resolves via clone + snapshot/HEAD. }, Locked: &projectconfig.ComponentLockData{ - UpstreamCommit: lockedCommit, + UpstreamCommit: "stale-locked-commit", }, }) - // No git expectations — locked commit should be returned without any clone. - identity, resolveErr := provider.ResolveIdentity(t.Context(), comp) - require.NoError(t, resolveErr) - assert.Equal(t, lockedCommit, identity, "should return locked commit without cloning") -} - -// TestFedoraProvider_ResolveIdentity_FallsBackWithoutLock verifies that when no -// Locked data is present, ResolveIdentity falls back to the old resolution path. -func TestFedoraProvider_ResolveIdentity_FallsBackWithoutLock(t *testing.T) { - ctrl := gomock.NewController(t) - mockGitProvider := git_test.NewMockGitProvider(ctrl) - - provider, err := sourceproviders.NewFedoraSourcesProviderImpl( - afero.NewMemMapFs(), - newNoOpDryRunnable(), - mockGitProvider, - newNoOpDownloader(), - testResolvedDistro(), - retry.Disabled(), - ) - require.NoError(t, err) - - headCommit := "head-fallback-commit" - - comp := newMockCompWithConfig(ctrl, testPackageName, &projectconfig.ComponentConfig{ - Name: testPackageName, - Spec: projectconfig.SpecSource{ - SourceType: projectconfig.SpecSourceTypeUpstream, - // No UpstreamCommit pin, no Locked data → falls back to clone + HEAD. - }, - }) - - // Expect clone + GetCurrentCommit (fallback path). + // Expects clone even though Locked data is present. mockGitProvider.EXPECT(). Clone(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). Return(nil) @@ -369,12 +340,14 @@ func TestFedoraProvider_ResolveIdentity_FallsBackWithoutLock(t *testing.T) { identity, resolveErr := provider.ResolveIdentity(t.Context(), comp) require.NoError(t, resolveErr) - assert.Equal(t, headCommit, identity, "should fall back to HEAD when no lock data") + assert.Equal(t, headCommit, identity, + "should resolve from upstream, ignoring locked data") } -// TestFedoraProvider_ResolveIdentity_LockedTakesPriorityOverConfigPin verifies -// that Locked.UpstreamCommit is used even when Spec.UpstreamCommit is also set. -func TestFedoraProvider_ResolveIdentity_LockedTakesPriorityOverConfigPin(t *testing.T) { +// TestFedoraProvider_ResolveIdentity_UsesConfigPin verifies that +// when Spec.UpstreamCommit is set, ResolveIdentity returns it +// directly (even if Locked data is also present). +func TestFedoraProvider_ResolveIdentity_UsesConfigPin(t *testing.T) { ctrl := gomock.NewController(t) mockGitProvider := git_test.NewMockGitProvider(ctrl) @@ -395,13 +368,13 @@ func TestFedoraProvider_ResolveIdentity_LockedTakesPriorityOverConfigPin(t *test UpstreamCommit: "config-pinned-commit", }, Locked: &projectconfig.ComponentLockData{ - UpstreamCommit: "locked-commit-wins", + UpstreamCommit: "locked-commit-ignored", }, }) - // No git expectations — locked commit returned directly. + // No clone expected — pinned commit returned directly. identity, resolveErr := provider.ResolveIdentity(t.Context(), comp) require.NoError(t, resolveErr) - assert.Equal(t, "locked-commit-wins", identity, - "locked commit should take priority over config pin for identity resolution") + assert.Equal(t, "config-pinned-commit", identity, + "config pin should be used for identity calculation") } diff --git a/scenario/__snapshots__/TestMCPServerMode_1.snap.json b/scenario/__snapshots__/TestMCPServerMode_1.snap.json index d9a49ff2..356765c2 100755 --- a/scenario/__snapshots__/TestMCPServerMode_1.snap.json +++ b/scenario/__snapshots__/TestMCPServerMode_1.snap.json @@ -181,7 +181,7 @@ "type": "boolean" }, "skip-lock-validation": { - "default": true, + "default": false, "description": "skip lock file consistency checks", "type": "boolean" }, @@ -270,11 +270,6 @@ "description": "only enable minimal output", "type": "boolean" }, - "skip-lock-validation": { - "default": true, - "description": "skip lock file consistency checks", - "type": "boolean" - }, "spec-path": { "description": "Spec path", "type": "string" diff --git a/scenario/component_build_test.go b/scenario/component_build_test.go index b245c1e9..59466ea6 100644 --- a/scenario/component_build_test.go +++ b/scenario/component_build_test.go @@ -127,7 +127,11 @@ func TestBuildingUpstreamComponent(t *testing.T) { ) // Run the build with test default configs copied into the container. - results := buildtest.NewBuildTest(project, testComponentName, projecttest.WithTestDefaultConfigs()).Run(t) + // component update populates lock files first (required by lock validation). + results := buildtest.NewBuildTest(project, testComponentName, + projecttest.WithTestDefaultConfigs(), + projecttest.WithPreCommand("component", "update", "-a"), + ).Run(t) // Make sure we got 1 SRPM. srpms := results.GetSRPMs(t) diff --git a/scenario/internal/projecttest/projecttest.go b/scenario/internal/projecttest/projecttest.go index c35158e8..c61bba51 100644 --- a/scenario/internal/projecttest/projecttest.go +++ b/scenario/internal/projecttest/projecttest.go @@ -12,6 +12,7 @@ import ( "strings" "testing" + "github.com/kballard/go-shellquote" "github.com/microsoft/azure-linux-dev-tools/scenario/internal/cmdtest" "github.com/microsoft/azure-linux-dev-tools/scenario/internal/containertest" "github.com/microsoft/azure-linux-dev-tools/scenario/internal/testhelpers" @@ -48,6 +49,7 @@ func TestDefaultConfigsDir() string { type ProjectTest struct { project TestProject commandArgs []string + preCommands [][]string useTestDefaultConfigs bool } @@ -63,6 +65,14 @@ func WithTestDefaultConfigs() ProjectTestOption { } } +// WithPreCommand adds a command to run before the main test command. Args are +// shell-quoted for safety. Each invocation is prefixed with 'azldev -C project -v'. +func WithPreCommand(args ...string) ProjectTestOption { + return func(p *ProjectTest) { + p.preCommands = append(p.preCommands, args) + } +} + // NewProjectTest creates a new [ProjectTest] with the specified project and command arguments. func NewProjectTest(project TestProject, commandArgs []string, options ...ProjectTestOption) *ProjectTest { params := &ProjectTest{ @@ -93,16 +103,22 @@ func (p *ProjectTest) RunInContainer(t *testing.T) *ProjectTestResults { p.project.Serialize(t, projectStagingDir) // Create a script that runs the command with the provided arguments. + var preCommandLines string + + for _, pre := range p.preCommands { + preCommandLines += "\nazldev -C project -v " + shellquote.Join(pre...) + } + testScript := fmt.Sprintf(` -set -x +set -ex # Ensure the build output dir is writable by mock; we accomplish this by symlinking # to the well-known dir created by mock for this purpose. rm -rf project/build ln -s /var/lib/mock project/build - +%s azldev -C project -v %s --output-format json >result.json -`, strings.Join(p.commandArgs, " ")) +`, preCommandLines, shellquote.Join(p.commandArgs...)) // NOTE: We need to run in a privileged container so 'mock' can create its nested root environment. // NOTE: We need to enable networking so 'mock' can download Azure Linux packages to build a root.