Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(repo-server): fixed repeated resource generation for source directory type #356

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions changelog/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
### Fixed
- fix: failures in update revision for path should not affect sync
### Changes
- fix(repo-server): fixed repeated resources generation for ApplicationSourceTypeDirectory
15 changes: 7 additions & 8 deletions reposerver/repository/repository.go
Original file line number Diff line number Diff line change
Expand Up @@ -1812,21 +1812,18 @@ var manifestFile = regexp.MustCompile(`^.*\.(yaml|yml|json|jsonnet)$`)

// findManifests looks at all yaml files in a directory and unmarshals them into a list of unstructured objects
func findManifests(logCtx *log.Entry, appPath string, repoRoot string, env *v1alpha1.Env, directory v1alpha1.ApplicationSourceDirectory, enabledManifestGeneration map[string]bool, maxCombinedManifestQuantity resource.Quantity) ([]manifest, error) {
absRepoRoot, err := filepath.Abs(repoRoot)
if err != nil {
return nil, err
}

var manifests []manifest

// Validate the directory before loading any manifests to save memory.
potentiallyValidManifests, err := getPotentiallyValidManifests(logCtx, appPath, repoRoot, directory.Recurse, directory.Include, directory.Exclude, maxCombinedManifestQuantity)
if err != nil {
logCtx.Errorf("failed to get potentially valid manifests: %s", err)
return nil, fmt.Errorf("failed to get potentially valid manifests: %w", err)
}

var objs []*unstructured.Unstructured
absRepoRoot, err := filepath.Abs(repoRoot)
if err != nil {
return nil, err
}
var manifests []manifest
for _, potentiallyValidManifest := range potentiallyValidManifests {
manifestPath := potentiallyValidManifest.path
manifestFileInfo := potentiallyValidManifest.fileInfo
Expand All @@ -1836,6 +1833,7 @@ func findManifests(logCtx *log.Entry, appPath string, repoRoot string, env *v1al
repoRelPath, _ := filepath.Rel(absRepoRoot, absPath)

if strings.HasSuffix(manifestFileInfo.Name(), ".jsonnet") {
var objs []*unstructured.Unstructured
if !discovery.IsManifestGenerationEnabled(v1alpha1.ApplicationSourceTypeDirectory, enabledManifestGeneration) {
continue
}
Expand Down Expand Up @@ -1875,6 +1873,7 @@ func findManifests(logCtx *log.Entry, appPath string, repoRoot string, env *v1al
})
}
} else {
var objs []*unstructured.Unstructured
err := getObjsFromYAMLOrJson(logCtx, manifestPath, manifestFileInfo.Name(), &objs)
if err != nil {
return nil, err
Expand Down
16 changes: 8 additions & 8 deletions reposerver/repository/repository_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -661,7 +661,7 @@ func TestGenerateManifestsUseExactRevision(t *testing.T) {

res1, err := service.GenerateManifest(context.Background(), &q)
require.NoError(t, err)
assert.Len(t, res1.Manifests, 6)
assert.Len(t, res1.Manifests, 4)
assert.Equal(t, "abc", gitClient.Calls[0].Arguments[0])
}

Expand All @@ -677,7 +677,7 @@ func TestRecurseManifestsInDir(t *testing.T) {

res1, err := service.GenerateManifest(context.Background(), &q)
require.NoError(t, err)
assert.Len(t, res1.Manifests, 6)
assert.Len(t, res1.Manifests, 4)
}

func TestInvalidManifestsInDir(t *testing.T) {
Expand Down Expand Up @@ -2199,7 +2199,7 @@ func TestFindResources(t *testing.T) {
}, {
name: "Include Everything",
include: "*.yaml",
expectedNames: []string{"nginx-deployment", "nginx-deployment", "nginx-deployment-sub"},
expectedNames: []string{"nginx-deployment", "nginx-deployment-sub"},
}, {
name: "Include Subdirectory",
include: "**/*.yaml",
Expand Down Expand Up @@ -2254,10 +2254,10 @@ func TestFindManifests_Exclude_NothingMatches(t *testing.T) {
}, map[string]bool{}, resource.MustParse("0"))

require.NoError(t, err)
require.Len(t, objs, 3)
require.Len(t, objs, 2)

assert.ElementsMatch(t,
[]string{"nginx-deployment", "nginx-deployment-sub"}, []string{objs[0].obj.GetName(), objs[2].obj.GetName()})
[]string{"nginx-deployment", "nginx-deployment-sub"}, []string{objs[0].obj.GetName(), objs[1].obj.GetName()})
}

func tempDir(t *testing.T) string {
Expand Down Expand Up @@ -2606,7 +2606,7 @@ func Test_findManifests(t *testing.T) {
t.Run("recursion when recursion is enabled", func(t *testing.T) {
recurse := argoappv1.ApplicationSourceDirectory{Recurse: true}
manifests, err := findManifests(logCtx, "./testdata/recurse", "./testdata/recurse", nil, recurse, nil, resource.MustParse("0"))
assert.Len(t, manifests, 6)
assert.Len(t, manifests, 4)
require.NoError(t, err)
})

Expand Down Expand Up @@ -2665,7 +2665,7 @@ func Test_findManifests(t *testing.T) {
t.Run("group of files should be limited at precisely the sum of their size", func(t *testing.T) {
// There is a total of 10 files, each file being 10 bytes.
manifests, err := findManifests(logCtx, "./testdata/several-files", "./testdata/several-files", nil, noRecurse, nil, resource.MustParse("365"))
assert.Len(t, manifests, 55)
assert.Len(t, manifests, 10)
require.NoError(t, err)

manifests, err = findManifests(logCtx, "./testdata/several-files", "./testdata/several-files", nil, noRecurse, nil, resource.MustParse("364"))
Expand All @@ -2676,7 +2676,7 @@ func Test_findManifests(t *testing.T) {
t.Run("jsonnet isn't counted against size limit", func(t *testing.T) {
// Each file is 36 bytes. Only the 36-byte json file should be counted against the limit.
manifests, err := findManifests(logCtx, "./testdata/jsonnet-and-json", "./testdata/jsonnet-and-json", nil, noRecurse, nil, resource.MustParse("36"))
assert.Len(t, manifests, 3)
assert.Len(t, manifests, 2)
require.NoError(t, err)

manifests, err = findManifests(logCtx, "./testdata/jsonnet-and-json", "./testdata/jsonnet-and-json", nil, noRecurse, nil, resource.MustParse("35"))
Expand Down
Loading