Config merge #982

godrei · 2024-07-05T08:37:50Z

Checklist

I've read and followed the Contribution Guidelines
README.md is updated with the changes (if needed)

Version

Context

This PR adds support for modular config files:

a new CLI command (merge) is introduced, which takes a modular (with config module includes) bitrise.yml file, merges the included modules to the main config file, and outputs the merged, final bitrise.yml and the config tree used for the merge.

$ bitrise merge -h

NAME:
   bitrise merge - Resolves includes in a modular bitrise.yml and merges included config modules into a single bitrise.yml file.

USAGE:
   bitrise merge [command options] args[0]: By default, the command looks for a bitrise.yml in the current directory, custom path can be specified as an argument.

OPTIONS:
   --output value, -o value  Output directory for the merged config file (bitrise.yml) and related config file tree (config_tree.json).

wires in modular config merge for all CLI commands working with the bitrise.yml file (run, trigger, trigger_check, validate, and workflows)

tothszabi · 2024-07-12T14:07:07Z

configmerge/configmerge.go

+	}
+}
+
+func (m *Merger) MergeConfig(mainConfigPth string) (string, *models.ConfigFileTreeModel, error) {


This Merger struct maybe could be a bit more higher level. It does not necessarily need to know if a config comes from a local file or remote one. What it needs to do is to read and understand the include section of the yamls, then ask an entity to return the bytes of the next yaml file, build a tree out of these and then merge it.

The other parts could be abstracted away a bit so you only know about them when you start digging deeper.

Created a new ConfigReader from the original FileReader and moved the local vs repo file read logic there.

tothszabi · 2024-07-12T14:08:49Z

configmerge/reference.go

+	includeCommit := r.Commit
+	isCommitValid := true
+	if includeCommit != "" {
+		isCommitValid = false
+
+		if len(includeCommit) > 5 && len(includeCommit) < 9 {
+			isCommitValid = true
+		} else if len(includeCommit) == 40 {
+			isCommitValid = true
+		}
+	}
+	if !isCommitValid {
+		return fmt.Errorf("invalid commit hash in reference (%s): %s", key, includeCommit)
+	}
+


Do we need to validate commit hashes or we can leave this job to the entity which tries to clone the repo?

This is a usual config model validation, to fail fast the merge process if the reference is in an invalid format.

tothszabi · 2024-07-12T14:10:20Z

configmerge/file_reader.go

+		opts.ReferenceName = plumbing.NewBranchReferenceName(branch)
+	}
+
+	repo, cloneErr := git.Clone(memory.NewStorage(), memfs.New(), &opts)


Would it make sense to introduce a local cache for the cloned repos? That way we do not need to perform the same action in the same merge command?

We agreed that a local file system cache should be used for a config merge process. This cache might be a folder in tmp dir and gets cleaned up, once the given bitrise.yml is resolved and a new merge for the same config would always start with a new cache.

tothszabi · 2024-07-12T14:11:55Z

configmerge/configmerge.go

+	sameRepo := false
+	if repoInfo != nil {
+		var err error
+		if sameRepo, err = isSameRepoReference(reference, *repoInfo); err != nil {
+			m.logger.Warnf("Failed to check if the reference is from the same repository: %s", err)
+		}
+	}
+
+	if sameRepo {
+		return m.readLocalConfigModule(reference, dir)
+	}


We discussed that this same repo logic could be removed because if the user enters a repository url then we just simply clone that one. Most probably if the user wants to refer to a file from the same branch/tag/commit then they will just simply enter a path value.

tothszabi · 2024-07-12T14:14:39Z

configmerge/configmerge.go

+	return &models.ConfigFileTreeModel{
+		Path:     key,
+		Contents: string(configContent),
+		Includes: includedConfigTrees,
+		Depth:    depth,
+	}, nil


Would it help if we flatten this tree before we actually merge it? If we flatten it to an array then the merge is simply iterating over the items one by one. And at the moment it is recursive ConfigFileTreeModel.Merge function.

I'm not sure if this would make the code simpler, because we anyway need to traverse the tree first.

…spects of reading a config module

tothszabi · 2024-07-26T08:39:54Z

cli/run_util.go

+			logger := logV2.NewLogger()
+			repoCache := configmerge.NewRepoCache()
+			configReader, err := configmerge.NewConfigReader(repoCache, logger)
+			if err != nil {
+				return models.BitriseDataModel{}, []string{}, fmt.Errorf("failed to create config module reader: %w", err)
+			}
+			merger := configmerge.NewMerger(configReader, logger)
+			mergedConfigContent, _, err := merger.MergeConfig(bitriseConfigPath)
+			if err != nil {
+				return models.BitriseDataModel{}, []string{}, fmt.Errorf("failed to merge Bitrise config (%s): %w", bitriseConfigPath, err)
+			}


This part is exactly the same as what can be found in the merge.go file. I would extract it in a common method so it is more visible that the same merge logic runs in both places.

Good idea, especially, because here I forgot to update the used logger package.
Fixed in 50e4b64

tothszabi · 2024-07-26T08:46:56Z

cli/run_util.go

 		if err != nil {
-			return models.BitriseDataModel{}, warnings, fmt.Errorf("Config (path:%s) is not valid: %s", bitriseConfigPath, err)
+			log.Warnf("Failed to check if the config is modular: %s", err)


How probably that if there is an error then that is recoverable? I looked at the IsModularConfig function and it can through errors if it has an I/O problem (cannot open or read the file) and when it cannot parse the yaml. Both of these are kinda fatal errors. I mean there is no harm not failing early here because that final config reading will fail as well if there is an I/O problem and there we bubble up the error.

Agree, updated in 9f58090

tothszabi · 2024-07-26T09:12:48Z

configmerge/repo_cache.go

+type repoCache struct {
+	cache map[string]string
+}
+
+func NewRepoCache() RepoCache {
+	return repoCache{
+		cache: map[string]string{},
+	}
+}
+
+func (c repoCache) GetRepo(ref ConfigReference) string {
+	return c.cache[ref.RepoKey()]
+}
+
+func (c repoCache) SetRepo(dir string, ref ConfigReference) {
+	c.cache[ref.RepoKey()] = dir
+}


Does it worth having this struct just to store a map? It seems too simple and this could move into the config reader.

If this entity would manage the local directory, folder creation and cleanup as well then I would say it is definitely a keeper. But in this state I think we can get rid of it.

Agree, updated in 8490578

tothszabi · 2024-07-26T09:15:26Z

models/config_file_tree.go

+	Contents string                `json:"contents,omitempty" yaml:"contents,omitempty"`
+	Includes []ConfigFileTreeModel `json:"includes,omitempty" yaml:"includes,omitempty"`
+
+	Depth int `json:"-" yaml:"-"`


Do we need this depth field? It seems like we save the depth in it but it is not used in a meaningful way. And it is ignored in both the json and yaml serialisation.

Good catch, thanks, this is not needed, removed in 5d2cc06

godrei added 30 commits July 5, 2024 09:33

Initialize modular config merge functionality

a503736

Introduce repoInfos

67fbe65

Check if included reference is a local reference

9925b34

Handle remote config modules

ed443f5

Support short commit hash in remote module reference

6987511

Merge branch 'master' into config-merge

22c2c41

Try ssh repo url format if http fails

dc89806

Code cleanup

bf823a8

Break configmerger dependencies

e6a7c99

Test config merge

1467012

Cache remote config modules

defc1fc

Test remote config module cache

0cabe7e

Adde expiration to config cache items

b0a495d

Code cleanup

d228b05

Remove config module cache support

1f42bd8

Merge config automatically for all commands

3d5cbf3

Update vendored deps and fix some e2e test

926d060

Update more e2e tests

70b44dc

Fix e2e tests

d8a0739

Code cleanup

901cc11

Validate config references

0d9d88f

Validate module config

e8a54c2

Test max include and max file count

d5d2b53

Test max file size

9fee75f

Cleanup bitrise utils

27f31b8

Fix merge command usage

2e721fa

Validate config modules earlier

1a258e1

Code cleanup

9d04fc1

Code cleanup and modular config integration tests

e9d0f84

merge command writes results to file

e553f42

godrei added 5 commits July 11, 2024 13:22

Add integration test for merge

89f06fa

Save merged configs to deploy dir

b964ff6

Fix modular config tests

e145918

Remove is CI condition from the auto config merge

35a65ee

Relative path reference test

2a4d574

godrei force-pushed the config-merge branch from 98a52d1 to 2a4d574 Compare July 11, 2024 12:39

Merge branch 'master' into config-merge

f21e495

tothszabi reviewed Jul 12, 2024

View reviewed changes

godrei added 12 commits July 12, 2024 16:43

Replace FileReader with a new ConfigReader, which encapsulates more a…

772b637

…spects of reading a config module

Remove current repo info handling from config merge

6cb08fa

Remove unused equalGitRepoURLs func

db24c8c

Merge branch 'master' into config-merge

1e429fb

Fix master rebase

33450c0

Move new reference validation to a new function

4ade429

Fix merge issue

ea6162d

Config module repository caching

39ab124

Cleanup repo cache and fix logging

2f51d87

Code cleanup

9c3c7b2

Code cleanup

510725e

Add remote config module include e2e test case

09a15b2

tothszabi reviewed Jul 26, 2024

View reviewed changes

godrei added 4 commits July 26, 2024 11:33

Remove unused depth field from config file tree

5d2cc06

Introduce createDefaultMerger func

50e4b64

Fail CreateBitriseConfigFromCLIParams on modular config check error

9f58090

Merge repo cache into fileReader

8490578

tothszabi approved these changes Jul 26, 2024

View reviewed changes

godrei merged commit 0026e43 into master Jul 26, 2024
5 checks passed

godrei deleted the config-merge branch July 26, 2024 13:32

This was referenced Jul 29, 2024

Modular config SchemaStore/schemastore#3955

Merged

Bump version to 2.19.0 #989

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Config merge #982

Config merge #982

godrei commented Jul 5, 2024 •

edited

Loading

tothszabi Jul 12, 2024

godrei Jul 14, 2024

tothszabi Jul 12, 2024

godrei Jul 14, 2024

tothszabi Jul 12, 2024

godrei Jul 14, 2024

tothszabi Jul 12, 2024

tothszabi Jul 12, 2024

godrei Jul 14, 2024

tothszabi Jul 26, 2024

godrei Jul 26, 2024

tothszabi Jul 26, 2024

godrei Jul 26, 2024

tothszabi Jul 26, 2024

godrei Jul 26, 2024

tothszabi Jul 26, 2024

godrei Jul 26, 2024

Config merge #982

Config merge #982

Conversation

godrei commented Jul 5, 2024 • edited Loading

Checklist

Version

Context

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

godrei commented Jul 5, 2024 •

edited

Loading