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

feat(appset): Support more than two child generators for matrix generator #14189

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 8 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 applicationset/controllers/requeue_after_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ func TestRequeueAfter(t *testing.T) {
"SCMProvider": terminalGenerators["SCMProvider"],
"ClusterDecisionResource": terminalGenerators["ClusterDecisionResource"],
"PullRequest": terminalGenerators["PullRequest"],
"Matrix": generators.NewMatrixGenerator(terminalGenerators),
"Matrix": generators.NewMatrixGenerator(terminalGenerators, 0),
"Merge": generators.NewMergeGenerator(terminalGenerators),
}

Expand All @@ -83,7 +83,7 @@ func TestRequeueAfter(t *testing.T) {
"SCMProvider": terminalGenerators["SCMProvider"],
"ClusterDecisionResource": terminalGenerators["ClusterDecisionResource"],
"PullRequest": terminalGenerators["PullRequest"],
"Matrix": generators.NewMatrixGenerator(nestedGenerators),
"Matrix": generators.NewMatrixGenerator(nestedGenerators, 0),
"Merge": generators.NewMergeGenerator(nestedGenerators),
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -354,7 +354,7 @@ func TestGetRelevantGenerators(t *testing.T) {
"Git": getMockGitGenerator(),
}

testGenerators["Matrix"] = NewMatrixGenerator(testGenerators)
testGenerators["Matrix"] = NewMatrixGenerator(testGenerators, 0)
testGenerators["Merge"] = NewMergeGenerator(testGenerators)
testGenerators["List"] = NewListGenerator()

Expand Down
63 changes: 35 additions & 28 deletions applicationset/generators/matrix.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,19 +15,21 @@ import (
var _ Generator = (*MatrixGenerator)(nil)

var (
ErrMoreThanTwoGenerators = fmt.Errorf("found more than two generators, Matrix support only two")
ErrLessThanTwoGenerators = fmt.Errorf("found less than two generators, Matrix support only two")
ErrMoreThenOneInnerGenerators = fmt.Errorf("found more than one generator in matrix.Generators")
ErrMoreThanMaxGenerators = fmt.Errorf("found more than the max allowed of child generators")
)

type MatrixGenerator struct {
// The inner generators supported by the matrix generator (cluster, git, list...)
supportedGenerators map[string]Generator
maxChildren int
}

func NewMatrixGenerator(supportedGenerators map[string]Generator) Generator {
func NewMatrixGenerator(supportedGenerators map[string]Generator, maxChildren int) Generator {
m := &MatrixGenerator{
supportedGenerators: supportedGenerators,
maxChildren: maxChildren,
}
return m
}
Expand All @@ -38,44 +40,49 @@ func (m *MatrixGenerator) GenerateParams(appSetGenerator *argoprojiov1alpha1.App
return nil, EmptyAppSetGeneratorError
}

if len(appSetGenerator.Matrix.Generators) < 2 {
numGens := len(appSetGenerator.Matrix.Generators)
if numGens < 2 {
return nil, ErrLessThanTwoGenerators
}

if len(appSetGenerator.Matrix.Generators) > 2 {
return nil, ErrMoreThanTwoGenerators
if m.maxChildren > 1 && numGens > m.maxChildren {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be useful to log a separate configuration error if value of maxChildren is 1 or less.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When maxChildren is 1 or less, it's considered no limit. The default value for maxChildren is 2.

Copy link
Member

@blakepettersson blakepettersson Jun 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a showstopper but that sounds un-intuitive IMO - could we not say that no limit only applies if maxChildren < 0?

Copy link
Author

@huynhsontung huynhsontung Jun 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to have maxChildren < 1 to avoid using negative values. The param reposerver.git.lsremote.parallelism.limit has the same logic where any values less than 1 mean unlimited.

plus we don't have to update the docs and tests this way

return nil, ErrMoreThanMaxGenerators
}

res := []map[string]interface{}{}

g0, err := m.getParams(appSetGenerator.Matrix.Generators[0], appSet, nil)
res, err := m.getParams(appSetGenerator.Matrix.Generators[0], appSet, nil)
if err != nil {
return nil, err
}
for _, a := range g0 {
g1, err := m.getParams(appSetGenerator.Matrix.Generators[1], appSet, a)
if err != nil {
return nil, fmt.Errorf("failed to get params for second generator in the matrix generator: %w", err)
}
for _, b := range g1 {

if appSet.Spec.GoTemplate {
tmp := map[string]interface{}{}
if err := mergo.Merge(&tmp, a); err != nil {
return nil, fmt.Errorf("failed to merge params from the first generator in the matrix generator with temp map: %w", err)
}
if err := mergo.Merge(&tmp, b); err != nil {
return nil, fmt.Errorf("failed to merge params from the first generator in the matrix generator with the second: %w", err)
}
res = append(res, tmp)
} else {
val, err := utils.CombineStringMaps(a, b)
if err != nil {
return nil, fmt.Errorf("failed to combine string maps with merging params for the matrix generator: %w", err)
for i := 1; i < numGens; i++ {
list := []map[string]interface{}{}
gen := appSetGenerator.Matrix.Generators[i]
for _, prevParam := range res {
params, err := m.getParams(gen, appSet, prevParam)
if err != nil {
return nil, fmt.Errorf("failed to get params for generators[%d] in the matrix generator: %w", i, err)
huynhsontung marked this conversation as resolved.
Show resolved Hide resolved
}
for _, currParam := range params {
if appSet.Spec.GoTemplate {
tmp := map[string]interface{}{}
if err := mergo.Merge(&tmp, prevParam); err != nil {
return nil, fmt.Errorf("failed to merge a previous params map with temp map in the matrix generator: %w", err)
}
if err := mergo.Merge(&tmp, currParam); err != nil {
return nil, fmt.Errorf("failed to merge a previous params map with params from generators[%d] in the matrix generator: %w", i, err)
huynhsontung marked this conversation as resolved.
Show resolved Hide resolved
}
list = append(list, tmp)
} else {
val, err := utils.CombineStringMaps(prevParam, currParam)
if err != nil {
return nil, fmt.Errorf("failed to combine string maps with merging params for the matrix generator: %w", err)
}
list = append(list, utils.ConvertToMapStringInterface(val))
}
res = append(res, utils.ConvertToMapStringInterface(val))
}
}

res = list
}

return res, nil
Expand Down
Loading