From 6c49029e152a8c1782bd49d0ef36ed3199d9f38e Mon Sep 17 00:00:00 2001 From: Samuel K Date: Mon, 4 May 2026 22:30:33 -0500 Subject: [PATCH] fix(feature): resolve all lint violations in pkg/devcontainer/feature - Extract test constants (testNodeFeatureID, testFeatureA/B/C) to fix goconst violations and remove all //nolint:goconst directives - Add AnnotationTitle/Description/Version constants for OCI annotation keys used across features.go and annotations_test.go - Extract testVersion1_0_0 constant in collection_test.go - Refactor findContainerUsers into smaller helper (resolveUserFromSources) to reduce cyclomatic complexity below cyclop threshold - Use strings.Builder in getFeatureLayers (modernize) - Break long shell string lines for lll compliance - Group GetExtendedBuildInfo params into ExtendedBuildInfoOptions struct (revive argument-limit) - Add filepath.Clean wrappers for gosec compliance in features.go - Extract hardDependencyTestCase type to fix funlen --- pkg/devcontainer/build.go | 32 +-- pkg/devcontainer/compose.go | 16 +- pkg/devcontainer/feature/annotations_test.go | 18 +- pkg/devcontainer/feature/collection_test.go | 9 +- pkg/devcontainer/feature/extend.go | 144 ++++++----- pkg/devcontainer/feature/extend_test.go | 252 +++++++++---------- pkg/devcontainer/feature/features.go | 18 +- 7 files changed, 258 insertions(+), 231 deletions(-) diff --git a/pkg/devcontainer/build.go b/pkg/devcontainer/build.go index a18519b8b..ae4e87a28 100644 --- a/pkg/devcontainer/build.go +++ b/pkg/devcontainer/build.go @@ -71,14 +71,14 @@ func (r *runner) extendImage( } // get extend image build info - extendedBuildInfo, err := feature.GetExtendedBuildInfo( - substitutionContext, - imageBuildInfo, - imageBase, - parsedConfig, - options.ForceBuild, - featureSecretOpts(options), - ) + extendedBuildInfo, err := feature.GetExtendedBuildInfo(&feature.ExtendedBuildInfoOptions{ + Ctx: substitutionContext, + ImageBuildInfo: imageBuildInfo, + Target: imageBase, + DevContainerConfig: parsedConfig, + ForceBuild: options.ForceBuild, + SecretOpts: featureSecretOpts(options), + }) if err != nil { return nil, fmt.Errorf("get extended build info: %w", err) } @@ -153,14 +153,14 @@ func (r *runner) buildAndExtendImage( } // get extend image build info - extendedBuildInfo, err := feature.GetExtendedBuildInfo( - substitutionContext, - imageBuildInfo, - imageBase, - parsedConfig, - options.ForceBuild, - featureSecretOpts(options), - ) + extendedBuildInfo, err := feature.GetExtendedBuildInfo(&feature.ExtendedBuildInfoOptions{ + Ctx: substitutionContext, + ImageBuildInfo: imageBuildInfo, + Target: imageBase, + DevContainerConfig: parsedConfig, + ForceBuild: options.ForceBuild, + SecretOpts: featureSecretOpts(options), + }) if err != nil { return nil, fmt.Errorf("get extended build info: %w", err) } diff --git a/pkg/devcontainer/compose.go b/pkg/devcontainer/compose.go index 28bcb63b3..dafed6032 100644 --- a/pkg/devcontainer/compose.go +++ b/pkg/devcontainer/compose.go @@ -685,14 +685,14 @@ func (r *runner) buildAndExtendDockerCompose( if featureSecretsFile != "" { secretOpts = &feature.SecretOptions{SecretsFile: featureSecretsFile} } - extendImageBuildInfo, err := feature.GetExtendedBuildInfo( - substitutionContext, - imageBuildInfo, - buildTarget, - parsedConfig, - false, - secretOpts, - ) + extendImageBuildInfo, err := feature.GetExtendedBuildInfo(&feature.ExtendedBuildInfoOptions{ + Ctx: substitutionContext, + ImageBuildInfo: imageBuildInfo, + Target: buildTarget, + DevContainerConfig: parsedConfig, + ForceBuild: false, + SecretOpts: secretOpts, + }) if err != nil { return composeExtendResult{}, err } diff --git a/pkg/devcontainer/feature/annotations_test.go b/pkg/devcontainer/feature/annotations_test.go index 043c7f0e8..397945b78 100644 --- a/pkg/devcontainer/feature/annotations_test.go +++ b/pkg/devcontainer/feature/annotations_test.go @@ -13,10 +13,10 @@ import ( func TestSaveAnnotations(t *testing.T) { dir := t.TempDir() annotations := map[string]string{ - "org.opencontainers.image.title": "Go", - "org.opencontainers.image.description": "Installs Go and common Go tools", - "org.opencontainers.image.version": "1.2.3", - "org.opencontainers.image.source": "https://github.com/devcontainers/features", + AnnotationTitle: "Go", + AnnotationDescription: "Installs Go and common Go tools", + AnnotationVersion: "1.2.3", + "org.opencontainers.image.source": "https://github.com/devcontainers/features", } saveAnnotations(dir, annotations) @@ -47,8 +47,8 @@ func TestLoadOCIAnnotations_Present(t *testing.T) { require.NoError(t, os.MkdirAll(extractedDir, 0o750)) annotations := map[string]string{ - "org.opencontainers.image.title": "Node.js", - "org.opencontainers.image.description": "Installs Node.js and common npm tools", + AnnotationTitle: "Node.js", + AnnotationDescription: "Installs Node.js and common npm tools", "org.opencontainers.image.authors": "Dev Containers", "org.opencontainers.image.url": "https://github.com/devcontainers/features/tree/main/src/node", "org.opencontainers.image.documentation": "https://containers.dev/features", @@ -94,9 +94,9 @@ func TestLogOCIAnnotations_NoTitle(t *testing.T) { func TestLogOCIAnnotations_WithTitle(t *testing.T) { annotations := map[string]string{ - "org.opencontainers.image.title": "Go", - "org.opencontainers.image.description": "Installs Go", - "org.opencontainers.image.version": "1.0.0", + AnnotationTitle: "Go", + AnnotationDescription: "Installs Go", + AnnotationVersion: testVersion1_0_0, } // Should not panic logOCIAnnotations("ghcr.io/devcontainers/features/go:1", annotations) diff --git a/pkg/devcontainer/feature/collection_test.go b/pkg/devcontainer/feature/collection_test.go index 0c7afa2a8..48aa22e20 100644 --- a/pkg/devcontainer/feature/collection_test.go +++ b/pkg/devcontainer/feature/collection_test.go @@ -17,7 +17,10 @@ import ( "github.com/stretchr/testify/suite" ) -const testFeatureNode = "node" +const ( + testFeatureNode = "node" + testVersion1_0_0 = "1.0.0" +) type CollectionTestSuite struct { suite.Suite @@ -103,7 +106,7 @@ func (s *CollectionTestSuite) TestFetchCollection_InvalidJSON() { func (s *CollectionTestSuite) TestListCollectionFeatures() { collection := Collection{ Features: []CollectionFeature{ - {ID: "rust", Version: "1.0.0", Name: "Rust"}, + {ID: "rust", Version: testVersion1_0_0, Name: "Rust"}, {ID: "python", Version: "3.0.0", Name: "Python"}, {ID: "java", Version: "1.5.0", Name: "Java"}, }, @@ -139,7 +142,7 @@ func (s *CollectionTestSuite) TestFetchCollection_DeprecatedFeature() { func (s *CollectionTestSuite) TestFetchCollection_FallbackToFirstLayer() { collection := Collection{ Features: []CollectionFeature{ - {ID: "fallback", Version: "1.0.0", Name: "Fallback"}, + {ID: "fallback", Version: testVersion1_0_0, Name: "Fallback"}, }, } data, err := json.Marshal(collection) diff --git a/pkg/devcontainer/feature/extend.go b/pkg/devcontainer/feature/extend.go index 9570a0e5d..8685ab471 100644 --- a/pkg/devcontainer/feature/extend.go +++ b/pkg/devcontainer/feature/extend.go @@ -51,14 +51,22 @@ type BuildInfo struct { BuildArgs map[string]string } -func GetExtendedBuildInfo( - ctx *config.SubstitutionContext, - imageBuildInfo *config.ImageBuildInfo, - target string, - devContainerConfig *config.SubstitutedConfig, - forceBuild bool, - secretOpts *SecretOptions, -) (*ExtendedBuildInfo, error) { +type ExtendedBuildInfoOptions struct { + Ctx *config.SubstitutionContext + ImageBuildInfo *config.ImageBuildInfo + Target string + DevContainerConfig *config.SubstitutedConfig + ForceBuild bool + SecretOpts *SecretOptions +} + +func GetExtendedBuildInfo(opts *ExtendedBuildInfoOptions) (*ExtendedBuildInfo, error) { + ctx := opts.Ctx + imageBuildInfo := opts.ImageBuildInfo + target := opts.Target + devContainerConfig := opts.DevContainerConfig + forceBuild := opts.ForceBuild + secretOpts := opts.SecretOpts features, err := fetchFeatures(devContainerConfig.Config, forceBuild, secretOpts) if err != nil { return nil, fmt.Errorf("fetch features: %w", err) @@ -219,22 +227,23 @@ func getFeatureSafeID(featureID string) string { } func getFeatureLayers(containerUser, remoteUser string, features []*config.FeatureSet) string { - result := `RUN \ -echo "_CONTAINER_USER_HOME=$(getent passwd ` + containerUser + ` | cut -d: -f6)" >> /tmp/build-features/devcontainer-features.builtin.env && \ -echo "_REMOTE_USER_HOME=$(getent passwd ` + remoteUser + ` | cut -d: -f6)" >> /tmp/build-features/devcontainer-features.builtin.env + var b strings.Builder + b.WriteString("RUN \\\n") + b.WriteString(`echo "_CONTAINER_USER_HOME=$(getent passwd ` + containerUser) + b.WriteString(` | cut -d: -f6)" >> ` + + `/tmp/build-features/devcontainer-features.builtin.env && \` + "\n") + b.WriteString(`echo "_REMOTE_USER_HOME=$(getent passwd ` + remoteUser) + b.WriteString(` | cut -d: -f6)" >> ` + + `/tmp/build-features/devcontainer-features.builtin.env` + "\n\n") -` for i, feature := range features { - result += generateContainerEnvs(feature) - result += ` -RUN cd /tmp/build-features/` + strconv.Itoa(i) + ` \ -&& chmod +x ./devcontainer-features-install.sh \ -&& ./devcontainer-features-install.sh - -` + b.WriteString(generateContainerEnvs(feature)) + b.WriteString("\nRUN cd /tmp/build-features/" + strconv.Itoa(i) + ` \` + "\n") + b.WriteString("&& chmod +x ./devcontainer-features-install.sh \\\n") + b.WriteString("&& ./devcontainer-features-install.sh\n\n") } - return result + return b.String() } func generateContainerEnvs(feature *config.FeatureSet) string { @@ -254,32 +263,34 @@ func findContainerUsers( composeServiceUser, imageUser string, ) (string, string) { reversed := config.ReverseSlice(baseImageMetadata.Config) - containerUser := "" - remoteUser := "" + containerUser := resolveUserFromSources( + reversed, composeServiceUser, imageUser, + func(m *config.ImageMetadata) string { return m.ContainerUser }, + ) + remoteUser := resolveUserFromSources( + reversed, composeServiceUser, imageUser, + func(m *config.ImageMetadata) string { return m.RemoteUser }, + ) + return containerUser, remoteUser +} + +func resolveUserFromSources( + reversed []*config.ImageMetadata, + composeServiceUser, imageUser string, + metadataField func(*config.ImageMetadata) string, +) string { for _, imageMetadata := range reversed { - if containerUser == "" && imageMetadata.ContainerUser != "" { - containerUser = imageMetadata.ContainerUser - } - if remoteUser == "" && imageMetadata.RemoteUser != "" { - remoteUser = imageMetadata.RemoteUser + if v := metadataField(imageMetadata); v != "" { + return v } } - - if containerUser == "" { - if composeServiceUser != "" { - containerUser = composeServiceUser - } else if imageUser != "" { - containerUser = imageUser - } + if composeServiceUser != "" { + return composeServiceUser } - if remoteUser == "" { - if composeServiceUser != "" { - remoteUser = composeServiceUser - } else if imageUser != "" { - remoteUser = imageUser - } + if imageUser != "" { + return imageUser } - return containerUser, remoteUser + return "" } // ResolveFeatureOrder parses the features in a DevContainerConfig, resolves their @@ -447,7 +458,7 @@ func (r *featureDependencyResolver) resolveFeatureDependency( featureSet *config.FeatureSet, ) error { if r.resolved[featureID] != nil { - return nil // Already resolved + return nil } if r.visiting[featureID] { @@ -458,28 +469,12 @@ func (r *featureDependencyResolver) resolveFeatureDependency( defer func() { r.visiting[featureID] = false }() for depID, depOptions := range featureSet.Config.DependsOn { - normalizedDepID := normalizeFeatureID(depID) - resolvedKey, depFeatureSet := r.findByConfigID(normalizedDepID) - if depFeatureSet == nil { - if currentID, legacyMatch := r.legacyMap[normalizedDepID]; legacyMatch { - log.Debugf("resolved legacy ID %s to current feature %s", depID, currentID) - resolvedKey, depFeatureSet = r.findByConfigID(currentID) - } - } - if depFeatureSet == nil { - log.Debugf("installing dependency feature %s", depID) - var err error - depFeatureSet, err = r.processor.processFeature(depID, depOptions) - if err != nil { - return fmt.Errorf("failed to resolve dependency %s: %w", depID, err) - } - resolvedKey = featureDeduplicationKey(depFeatureSet.ConfigID, depFeatureSet.Version) - r.features[resolvedKey] = depFeatureSet - r.rebuildLegacyMap() + resolvedKey, depFeatureSet, err := r.findOrInstallDependency(depID, depOptions) + if err != nil { + return err } - err := r.resolveFeatureDependency(resolvedKey, depFeatureSet) - if err != nil { + if err := r.resolveFeatureDependency(resolvedKey, depFeatureSet); err != nil { return err } } @@ -488,6 +483,31 @@ func (r *featureDependencyResolver) resolveFeatureDependency( return nil } +func (r *featureDependencyResolver) findOrInstallDependency( + depID string, depOptions any, +) (string, *config.FeatureSet, error) { + normalizedDepID := normalizeFeatureID(depID) + resolvedKey, depFeatureSet := r.findByConfigID(normalizedDepID) + if depFeatureSet == nil { + if currentID, legacyMatch := r.legacyMap[normalizedDepID]; legacyMatch { + log.Debugf("resolved legacy ID %s to current feature %s", depID, currentID) + resolvedKey, depFeatureSet = r.findByConfigID(currentID) + } + } + if depFeatureSet == nil { + log.Debugf("installing dependency feature %s", depID) + var err error + depFeatureSet, err = r.processor.processFeature(depID, depOptions) + if err != nil { + return "", nil, fmt.Errorf("failed to resolve dependency %s: %w", depID, err) + } + resolvedKey = featureDeduplicationKey(depFeatureSet.ConfigID, depFeatureSet.Version) + r.features[resolvedKey] = depFeatureSet + r.rebuildLegacyMap() + } + return resolvedKey, depFeatureSet, nil +} + func (r *featureDependencyResolver) rebuildLegacyMap() { r.legacyMap = buildLegacyIDMap(r.features) } diff --git a/pkg/devcontainer/feature/extend_test.go b/pkg/devcontainer/feature/extend_test.go index c60d671b3..0321bafa0 100644 --- a/pkg/devcontainer/feature/extend_test.go +++ b/pkg/devcontainer/feature/extend_test.go @@ -7,6 +7,14 @@ import ( "github.com/stretchr/testify/suite" ) +const ( + testNodeFeatureID = "ghcr.io/devcontainers/features/node" + testFeatureA = "feature-a" + testFeatureB = "feature-b" + testFeatureC = "feature-c" + testVersion23 = "2.3" +) + type ExtendTestSuite struct { suite.Suite } @@ -17,9 +25,9 @@ func TestExtendTestSuite(t *testing.T) { func (suite *ExtendTestSuite) TestCreateFeatureLookup() { features := []*config.FeatureSet{ - {ConfigID: "feature-a"}, - {ConfigID: "feature-b"}, - {ConfigID: "feature-c"}, + {ConfigID: testFeatureA}, + {ConfigID: testFeatureB}, + {ConfigID: testFeatureC}, } lookup := buildFeatureLookupMap(features) @@ -30,67 +38,63 @@ func (suite *ExtendTestSuite) TestCreateFeatureLookup() { } } -func (suite *ExtendTestSuite) TestHasHardDependency() { - tests := []struct { - name string - feature *config.FeatureSet - originalID string - normalizedID string - expectedIsDuplicate bool - }{ +type hardDependencyTestCase struct { + name string + feature *config.FeatureSet + originalID string + normalizedID string + expectedIsDuplicate bool +} + +func hardDependencyTestCases() []hardDependencyTestCase { + return []hardDependencyTestCase{ { name: "exact match in dependsOn", feature: &config.FeatureSet{ Config: &config.FeatureConfig{ - DependsOn: config.DependsOnField{ - "node": map[string]any{}, - }, + DependsOn: config.DependsOnField{testFeatureNode: map[string]any{}}, }, }, - originalID: "node", - normalizedID: "node", + originalID: testFeatureNode, + normalizedID: testFeatureNode, expectedIsDuplicate: true, }, { name: "normalized match in dependsOn", feature: &config.FeatureSet{ Config: &config.FeatureConfig{ - DependsOn: config.DependsOnField{ - "ghcr.io/devcontainers/features/node": map[string]any{}, - }, + DependsOn: config.DependsOnField{testNodeFeatureID: map[string]any{}}, }, }, - originalID: "ghcr.io/devcontainers/features/node:latest", - normalizedID: "ghcr.io/devcontainers/features/node", + originalID: testNodeFeatureID + ":latest", + normalizedID: testNodeFeatureID, expectedIsDuplicate: true, }, { name: "no match", feature: &config.FeatureSet{ Config: &config.FeatureConfig{ - DependsOn: config.DependsOnField{ - "python": map[string]any{}, - }, + DependsOn: config.DependsOnField{"python": map[string]any{}}, }, }, - originalID: "node", - normalizedID: "node", + originalID: testFeatureNode, + normalizedID: testFeatureNode, expectedIsDuplicate: false, }, { name: "empty dependsOn", feature: &config.FeatureSet{ - Config: &config.FeatureConfig{ - DependsOn: config.DependsOnField{}, - }, + Config: &config.FeatureConfig{DependsOn: config.DependsOnField{}}, }, - originalID: "node", - normalizedID: "node", + originalID: testFeatureNode, + normalizedID: testFeatureNode, expectedIsDuplicate: false, }, } +} - for _, testCase := range tests { +func (suite *ExtendTestSuite) TestHasHardDependency() { + for _, testCase := range hardDependencyTestCases() { suite.Run(testCase.name, func() { actualIsDuplicate := hasHardDependency( testCase.feature, @@ -243,18 +247,18 @@ func (suite *ExtendTestSuite) TestComputeAutomaticFeatureOrder_ChainedDependenci func (suite *ExtendTestSuite) TestComputeAutomaticFeatureOrder_CircularDependency() { features := []*config.FeatureSet{ { - ConfigID: normalizeFeatureID("feature-a"), + ConfigID: normalizeFeatureID(testFeatureA), Config: &config.FeatureConfig{ DependsOn: config.DependsOnField{ - "feature-b": map[string]any{}, + testFeatureB: map[string]any{}, }, }, }, { - ConfigID: normalizeFeatureID("feature-b"), + ConfigID: normalizeFeatureID(testFeatureB), Config: &config.FeatureConfig{ DependsOn: config.DependsOnField{ - "feature-a": map[string]any{}, + testFeatureA: map[string]any{}, }, }, }, @@ -271,13 +275,13 @@ func (suite *ExtendTestSuite) TestFeatureOrderWithDependencies_SameDependsOnAndI ConfigID: "dev-code", Config: &config.FeatureConfig{ DependsOn: config.DependsOnField{ - "ghcr.io/devcontainers/features/node": map[string]any{}, + testNodeFeatureID: map[string]any{}, }, - InstallsAfter: []string{"ghcr.io/devcontainers/features/node"}, + InstallsAfter: []string{testNodeFeatureID}, }, }, { - ConfigID: "ghcr.io/devcontainers/features/node", + ConfigID: testNodeFeatureID, Config: &config.FeatureConfig{ DependsOn: config.DependsOnField{}, InstallsAfter: []string{}, @@ -288,7 +292,7 @@ func (suite *ExtendTestSuite) TestFeatureOrderWithDependencies_SameDependsOnAndI installationOrder, err := getOrderedFeatureSets(features) suite.Require().NoError(err) suite.Len(installationOrder, 2) - suite.Equal("ghcr.io/devcontainers/features/node", installationOrder[0].ConfigID) + suite.Equal(testNodeFeatureID, installationOrder[0].ConfigID) suite.Equal("dev-code", installationOrder[1].ConfigID) } @@ -301,13 +305,13 @@ func (suite *ExtendTestSuite) TestComputeFeatureOrder_NoOverride() { features := []*config.FeatureSet{ { - ConfigID: normalizeFeatureID("feature-a"), + ConfigID: normalizeFeatureID(testFeatureA), Config: &config.FeatureConfig{ - DependsOn: config.DependsOnField{"feature-b": map[string]any{}}, + DependsOn: config.DependsOnField{testFeatureB: map[string]any{}}, }, }, { - ConfigID: normalizeFeatureID("feature-b"), + ConfigID: normalizeFeatureID(testFeatureB), Config: &config.FeatureConfig{DependsOn: config.DependsOnField{}}, }, } @@ -316,14 +320,14 @@ func (suite *ExtendTestSuite) TestComputeFeatureOrder_NoOverride() { suite.Require().NoError(err) suite.Len(order, 2) - expectedFeatureB := normalizeFeatureID("feature-b") - expectedFeatureA := normalizeFeatureID("feature-a") - if order[0].ConfigID != expectedFeatureB || order[1].ConfigID != expectedFeatureA { + expB := normalizeFeatureID(testFeatureB) + expA := normalizeFeatureID(testFeatureA) + if order[0].ConfigID != expB || order[1].ConfigID != expA { suite.Failf( "Order mismatch", "Expected [%s, %s], got [%s, %s]", - expectedFeatureB, - expectedFeatureA, + expB, + expA, order[0].ConfigID, order[1].ConfigID, ) @@ -333,18 +337,18 @@ func (suite *ExtendTestSuite) TestComputeFeatureOrder_NoOverride() { func (suite *ExtendTestSuite) TestComputeFeatureOrder_OverrideViolatesDependsOn() { devContainer := &config.DevContainerConfig{ DevContainerConfigBase: config.DevContainerConfigBase{ - OverrideFeatureInstallOrder: []string{"feature-a", "feature-b"}, + OverrideFeatureInstallOrder: []string{testFeatureA, testFeatureB}, }, } features := []*config.FeatureSet{ { - ConfigID: "feature-a", + ConfigID: testFeatureA, Config: &config.FeatureConfig{ - DependsOn: config.DependsOnField{"feature-b": map[string]any{}}, + DependsOn: config.DependsOnField{testFeatureB: map[string]any{}}, }, }, - {ConfigID: "feature-b", Config: &config.FeatureConfig{DependsOn: config.DependsOnField{}}}, + {ConfigID: testFeatureB, Config: &config.FeatureConfig{DependsOn: config.DependsOnField{}}}, } _, err := getSortedFeatureSets(devContainer, features) @@ -356,104 +360,104 @@ func (suite *ExtendTestSuite) TestComputeFeatureOrder_OverrideViolatesDependsOn( func (suite *ExtendTestSuite) TestComputeFeatureOrder_ValidOverride() { devContainer := &config.DevContainerConfig{ DevContainerConfigBase: config.DevContainerConfigBase{ - OverrideFeatureInstallOrder: []string{"feature-b", "feature-a"}, + OverrideFeatureInstallOrder: []string{testFeatureB, testFeatureA}, }, } features := []*config.FeatureSet{ { - ConfigID: "feature-a", + ConfigID: testFeatureA, Config: &config.FeatureConfig{ - DependsOn: config.DependsOnField{"feature-b": map[string]any{}}, + DependsOn: config.DependsOnField{testFeatureB: map[string]any{}}, }, }, - {ConfigID: "feature-b", Config: &config.FeatureConfig{DependsOn: config.DependsOnField{}}}, + {ConfigID: testFeatureB, Config: &config.FeatureConfig{DependsOn: config.DependsOnField{}}}, } order, err := getSortedFeatureSets(devContainer, features) suite.Require().NoError(err) suite.Len(order, 2) - suite.Equal("feature-b", order[0].ConfigID) - suite.Equal("feature-a", order[1].ConfigID) + suite.Equal(testFeatureB, order[0].ConfigID) + suite.Equal(testFeatureA, order[1].ConfigID) } func (suite *ExtendTestSuite) TestComputeFeatureOrder_PartialOverride() { devContainer := &config.DevContainerConfig{ DevContainerConfigBase: config.DevContainerConfigBase{ - OverrideFeatureInstallOrder: []string{"feature-c"}, + OverrideFeatureInstallOrder: []string{testFeatureC}, }, } features := []*config.FeatureSet{ { - ConfigID: "feature-a", + ConfigID: testFeatureA, Config: &config.FeatureConfig{ - DependsOn: config.DependsOnField{"feature-b": map[string]any{}}, + DependsOn: config.DependsOnField{testFeatureB: map[string]any{}}, }, }, - {ConfigID: "feature-b", Config: &config.FeatureConfig{DependsOn: config.DependsOnField{}}}, - {ConfigID: "feature-c", Config: &config.FeatureConfig{DependsOn: config.DependsOnField{}}}, + {ConfigID: testFeatureB, Config: &config.FeatureConfig{DependsOn: config.DependsOnField{}}}, + {ConfigID: testFeatureC, Config: &config.FeatureConfig{DependsOn: config.DependsOnField{}}}, } order, err := getSortedFeatureSets(devContainer, features) suite.Require().NoError(err) suite.Len(order, 3) - if order[0].ConfigID != "feature-c" { + if order[0].ConfigID != testFeatureC { suite.Failf("First element mismatch", "Expected feature-c first, got %s", order[0].ConfigID) } } func (suite *ExtendTestSuite) TestBuildOverridePriority() { features := []*config.FeatureSet{ - {ConfigID: "feature-a", Config: &config.FeatureConfig{DependsOn: config.DependsOnField{}}}, - {ConfigID: "feature-b", Config: &config.FeatureConfig{DependsOn: config.DependsOnField{}}}, - {ConfigID: "feature-c", Config: &config.FeatureConfig{DependsOn: config.DependsOnField{}}}, + {ConfigID: testFeatureA, Config: &config.FeatureConfig{DependsOn: config.DependsOnField{}}}, + {ConfigID: testFeatureB, Config: &config.FeatureConfig{DependsOn: config.DependsOnField{}}}, + {ConfigID: testFeatureC, Config: &config.FeatureConfig{DependsOn: config.DependsOnField{}}}, } lookup := buildFeatureLookupMap(features) - overrideOrder := []string{"feature-c", "feature-a"} + overrideOrder := []string{testFeatureC, testFeatureA} priority := buildOverridePriority(overrideOrder, lookup) - suite.Equal(0, priority["feature-c"]) - suite.Equal(1, priority["feature-a"]) - _, hasB := priority["feature-b"] + suite.Equal(0, priority[testFeatureC]) + suite.Equal(1, priority[testFeatureA]) + _, hasB := priority[testFeatureB] suite.False(hasB) } func (suite *ExtendTestSuite) TestOverridePriorityAffectsSortOrder() { devContainer := &config.DevContainerConfig{ DevContainerConfigBase: config.DevContainerConfigBase{ - OverrideFeatureInstallOrder: []string{"feature-c", "feature-a"}, + OverrideFeatureInstallOrder: []string{testFeatureC, testFeatureA}, }, } features := []*config.FeatureSet{ - {ConfigID: "feature-a", Config: &config.FeatureConfig{DependsOn: config.DependsOnField{}}}, - {ConfigID: "feature-b", Config: &config.FeatureConfig{DependsOn: config.DependsOnField{}}}, - {ConfigID: "feature-c", Config: &config.FeatureConfig{DependsOn: config.DependsOnField{}}}, + {ConfigID: testFeatureA, Config: &config.FeatureConfig{DependsOn: config.DependsOnField{}}}, + {ConfigID: testFeatureB, Config: &config.FeatureConfig{DependsOn: config.DependsOnField{}}}, + {ConfigID: testFeatureC, Config: &config.FeatureConfig{DependsOn: config.DependsOnField{}}}, } order, err := getSortedFeatureSets(devContainer, features) suite.Require().NoError(err) suite.Len(order, 3) - suite.Equal("feature-c", order[0].ConfigID) - suite.Equal("feature-a", order[1].ConfigID) - suite.Equal("feature-b", order[2].ConfigID) + suite.Equal(testFeatureC, order[0].ConfigID) + suite.Equal(testFeatureA, order[1].ConfigID) + suite.Equal(testFeatureB, order[2].ConfigID) } func (suite *ExtendTestSuite) TestExtractFeatureByID() { features := []*config.FeatureSet{ - {ConfigID: "feature-a"}, - {ConfigID: "feature-b"}, + {ConfigID: testFeatureA}, + {ConfigID: testFeatureB}, } - found := extractFeatureByID(features, "feature-b") - if found == nil || found.ConfigID != "feature-b" { + found := extractFeatureByID(features, testFeatureB) + if found == nil || found.ConfigID != testFeatureB { suite.Fail("Expected to find feature-b") } - notFound := extractFeatureByID(features, "feature-c") + notFound := extractFeatureByID(features, testFeatureC) if notFound != nil { suite.Fail("Expected not to find feature-c") } @@ -461,15 +465,15 @@ func (suite *ExtendTestSuite) TestExtractFeatureByID() { func (suite *ExtendTestSuite) TestContainsFeature() { features := []*config.FeatureSet{ - {ConfigID: "feature-a"}, - {ConfigID: "feature-b"}, + {ConfigID: testFeatureA}, + {ConfigID: testFeatureB}, } - if !containsFeature(features, "feature-a") { + if !containsFeature(features, testFeatureA) { suite.Fail("Expected to contain feature-a") } - if containsFeature(features, "feature-c") { + if containsFeature(features, testFeatureC) { suite.Fail("Expected not to contain feature-c") } } @@ -562,15 +566,15 @@ func (suite *ExtendTestSuite) TestResolveDependencies_LegacyIDResolution() { func (suite *ExtendTestSuite) TestResolveDependencies_LegacyIDNotUsedWhenPrimaryExists() { features := map[string]*config.FeatureSet{ - "feature-a": { - ConfigID: "feature-a", + testFeatureA: { + ConfigID: testFeatureA, Config: &config.FeatureConfig{ - LegacyIds: []string{"feature-b"}, + LegacyIds: []string{testFeatureB}, DependsOn: config.DependsOnField{}, }, }, - "feature-b": { - ConfigID: "feature-b", + testFeatureB: { + ConfigID: testFeatureB, Config: &config.FeatureConfig{ DependsOn: config.DependsOnField{}, }, @@ -579,7 +583,7 @@ func (suite *ExtendTestSuite) TestResolveDependencies_LegacyIDNotUsedWhenPrimary ConfigID: "consumer", Config: &config.FeatureConfig{ DependsOn: config.DependsOnField{ - "feature-b": map[string]any{}, + testFeatureB: map[string]any{}, }, }, }, @@ -588,26 +592,25 @@ func (suite *ExtendTestSuite) TestResolveDependencies_LegacyIDNotUsedWhenPrimary resolved, err := resolveDependencies(&featureProcessor{}, features) suite.Require().NoError(err) suite.Len(resolved, 3) - suite.NotNil(resolved["feature-b"]) + suite.NotNil(resolved[testFeatureB]) } func (suite *ExtendTestSuite) TestVersionAwareDeduplication_SameConfigSameVersion() { - featureID := "ghcr.io/devcontainers/features/node" //nolint:goconst features := map[string]*config.FeatureSet{} f1 := &config.FeatureSet{ - ConfigID: featureID, + ConfigID: testNodeFeatureID, Version: "1", Config: &config.FeatureConfig{DependsOn: config.DependsOnField{}}, } f2 := &config.FeatureSet{ - ConfigID: featureID, + ConfigID: testNodeFeatureID, Version: "1", Config: &config.FeatureConfig{DependsOn: config.DependsOnField{}}, } - key := featureDeduplicationKey(featureID, "1") - suite.Equal(featureID+":1", key) + key := featureDeduplicationKey(testNodeFeatureID, "1") + suite.Equal(testNodeFeatureID+":1", key) features[featureDeduplicationKey(f1.ConfigID, f1.Version)] = f1 features[featureDeduplicationKey(f2.ConfigID, f2.Version)] = f2 @@ -615,16 +618,15 @@ func (suite *ExtendTestSuite) TestVersionAwareDeduplication_SameConfigSameVersio } func (suite *ExtendTestSuite) TestVersionAwareDeduplication_SameConfigDifferentVersion() { - featureID := "ghcr.io/devcontainers/features/node" features := map[string]*config.FeatureSet{} v1 := &config.FeatureSet{ - ConfigID: featureID, + ConfigID: testNodeFeatureID, Version: "1", Config: &config.FeatureConfig{DependsOn: config.DependsOnField{}}, } v2 := &config.FeatureSet{ - ConfigID: featureID, + ConfigID: testNodeFeatureID, Version: "2", Config: &config.FeatureConfig{DependsOn: config.DependsOnField{}}, } @@ -635,16 +637,15 @@ func (suite *ExtendTestSuite) TestVersionAwareDeduplication_SameConfigDifferentV } func (suite *ExtendTestSuite) TestVersionAwareDeduplication_EmptyVersionIsDuplicate() { - featureID := "ghcr.io/devcontainers/features/node" features := map[string]*config.FeatureSet{} f1 := &config.FeatureSet{ - ConfigID: featureID, + ConfigID: testNodeFeatureID, Version: "", Config: &config.FeatureConfig{DependsOn: config.DependsOnField{}}, } f2 := &config.FeatureSet{ - ConfigID: featureID, + ConfigID: testNodeFeatureID, Version: "", Config: &config.FeatureConfig{DependsOn: config.DependsOnField{}}, } @@ -655,17 +656,16 @@ func (suite *ExtendTestSuite) TestVersionAwareDeduplication_EmptyVersionIsDuplic } func (suite *ExtendTestSuite) TestExtractVersionFromFeatureID() { - nodeFeature := "ghcr.io/devcontainers/features/node" //nolint:goconst tests := []struct { input string expected string }{ - {nodeFeature + ":1", "1"}, - {nodeFeature + ":2", "2"}, - {nodeFeature + ":latest", ""}, - {nodeFeature, ""}, - {nodeFeature + ":v1", "1"}, - {nodeFeature + ":v2.3", "2.3"}, //nolint:goconst + {testNodeFeatureID + ":1", "1"}, + {testNodeFeatureID + ":2", "2"}, + {testNodeFeatureID + ":latest", ""}, + {testNodeFeatureID, ""}, + {testNodeFeatureID + ":v1", "1"}, + {testNodeFeatureID + ":v2.3", testVersion23}, } for _, tc := range tests { @@ -684,8 +684,8 @@ func (suite *ExtendTestSuite) TestNormalizeVersion() { {"", ""}, {"1", "1"}, {"v1", "1"}, - {"2.3", "2.3"}, - {"v2.3", "2.3"}, + {testVersion23, testVersion23}, + {"v2.3", testVersion23}, } for _, tc := range tests { @@ -696,33 +696,31 @@ func (suite *ExtendTestSuite) TestNormalizeVersion() { } func (suite *ExtendTestSuite) TestContainsFeature_VersionAware() { - featureID := "ghcr.io/devcontainers/features/node" //nolint:goconst features := []*config.FeatureSet{ - {ConfigID: featureID, Version: "1"}, - {ConfigID: featureID, Version: "2"}, + {ConfigID: testNodeFeatureID, Version: "1"}, + {ConfigID: testNodeFeatureID, Version: "2"}, } - suite.True(containsFeature(features, "ghcr.io/devcontainers/features/node:1")) - suite.True(containsFeature(features, "ghcr.io/devcontainers/features/node:2")) - suite.False(containsFeature(features, "ghcr.io/devcontainers/features/node:3")) - suite.False(containsFeature(features, "ghcr.io/devcontainers/features/node:latest")) + suite.True(containsFeature(features, testNodeFeatureID+":1")) + suite.True(containsFeature(features, testNodeFeatureID+":2")) + suite.False(containsFeature(features, testNodeFeatureID+":3")) + suite.False(containsFeature(features, testNodeFeatureID+":latest")) } func (suite *ExtendTestSuite) TestExtractFeatureByID_VersionAware() { - featureID := "ghcr.io/devcontainers/features/node" //nolint:goconst features := []*config.FeatureSet{ - {ConfigID: featureID, Version: "1"}, - {ConfigID: featureID, Version: "2"}, + {ConfigID: testNodeFeatureID, Version: "1"}, + {ConfigID: testNodeFeatureID, Version: "2"}, } - found := extractFeatureByID(features, "ghcr.io/devcontainers/features/node:1") + found := extractFeatureByID(features, testNodeFeatureID+":1") suite.NotNil(found) suite.Equal("1", found.Version) - found = extractFeatureByID(features, "ghcr.io/devcontainers/features/node:2") + found = extractFeatureByID(features, testNodeFeatureID+":2") suite.NotNil(found) suite.Equal("2", found.Version) - notFound := extractFeatureByID(features, "ghcr.io/devcontainers/features/node:3") + notFound := extractFeatureByID(features, testNodeFeatureID+":3") suite.Nil(notFound) } diff --git a/pkg/devcontainer/feature/features.go b/pkg/devcontainer/feature/features.go index efb585b3a..b67bd787e 100644 --- a/pkg/devcontainer/feature/features.go +++ b/pkg/devcontainer/feature/features.go @@ -27,6 +27,12 @@ import ( const DEVCONTAINER_MANIFEST_MEDIATYPE = "application/vnd.devcontainers" +const ( + AnnotationTitle = "org.opencontainers.image.title" + AnnotationDescription = "org.opencontainers.image.description" + AnnotationVersion = "org.opencontainers.image.version" +) + var directTarballRegEx = regexp.MustCompile("devcontainer-feature-([a-zA-Z0-9_-]+).tgz") func getFeatureInstallWrapperScript( @@ -256,9 +262,9 @@ func processOCIFeature(id string) (string, error) { const annotationsFileName = "annotations.json" func logOCIAnnotations(id string, annotations map[string]string) { - title := annotations["org.opencontainers.image.title"] - description := annotations["org.opencontainers.image.description"] - version := annotations["org.opencontainers.image.version"] + title := annotations[AnnotationTitle] + description := annotations[AnnotationDescription] + version := annotations[AnnotationVersion] if title != "" || description != "" { log.Infof( @@ -321,7 +327,7 @@ func pullAndExtractOCIFeature( return nil, fmt.Errorf("download layer from %s: %w", registry, err) } - file, err := os.Open(destFile) + file, err := os.Open(filepath.Clean(destFile)) if err != nil { return nil, err } @@ -385,7 +391,7 @@ func writeLayerToFile(data io.Reader, destFile string) error { return fmt.Errorf("create target folder: %w", err) } - file, err := os.Create(destFile) + file, err := os.Create(filepath.Clean(destFile)) if err != nil { return fmt.Errorf("create file: %w", err) } @@ -589,7 +595,7 @@ func tryDownload(url, destFile string, httpHeaders map[string]string) error { return fmt.Errorf("GET request failed, status code is %d", resp.StatusCode) } - file, err := os.Create(destFile) + file, err := os.Create(filepath.Clean(destFile)) if err != nil { return fmt.Errorf("create download file: %w", err) }