internal/config: include remote configuration files #96

seilagamo · 2024-09-19T14:11:47Z

This PR build a Direct Acyclic Graph while is discovering the includes in the configuration files recursively.

This PR DOES NOT merge the configurations yet.

Note that this PR is strongly inspired by https://github.com/KyleBanks/depth.

internal/config/config.go

internal/dag/dag.go

internal/config/include.go

jroimartin · 2025-01-17T12:21:57Z

.github/workflows/golangci-lint.yaml

-      GOLANGCI_LINT_VERSION: v1.57.2
-      GOLANGCI_LINT_OUT_FORMAT: ${{ github.event_name == 'pull_request' && 'github-actions' || 'colored-line-number' }}
+      GOLANGCI_LINT_VERSION: v1.63.4
+      GOLANGCI_LINT_OUT_FORMAT: 'colored-line-number'


Why did you change this? IIRC this setting allowed to get inline error messages in pull requests while avoiding duplicates because of runs on push.

Because of this warning message: https://github.com/adevinta/lava/actions/runs/12827986995/job/35771096163

jroimartin · 2025-01-17T12:36:38Z

internal/config/config.go

@@ -83,12 +86,33 @@ type Config struct {
 var reEnv = regexp.MustCompile(`\$\{[a-zA-Z_][a-zA-Z_0-9]*\}`)

 // Parse returns a parsed Lava configuration given an [io.Reader].
-func Parse(r io.Reader) (Config, error) {
+func Parse(r io.Reader, path string) (Config, error) {


I don't like this signature at all. If we always pass a file path, and given that this is an internal API, let's have only Parse that takes the file path.

func Parse(path string) (Config, error) { ... }

It also removes the problem of having an empty path when generating the DAG. We can even avoid the first decoding and let config.DAG to do the job.

jroimartin · 2025-01-17T12:40:31Z

internal/config/dag/dag.go

+// AddVertex adds the vertex x to the DAG. AddVertex returns an error if s is
+// nil, s is already part of the graph.


Suggested change

// AddVertex adds the vertex x to the DAG. AddVertex returns an error if s is

// nil, s is already part of the graph.

// AddVertex adds the vertex x to the DAG. AddVertex returns an error if s is

// nil or s is already part of the graph.

jroimartin · 2025-01-17T12:41:50Z

internal/config/dag/dag.go

+}
+
+// NewDAG returns a Directed Acyclic Graph object.
+func NewDAG() *DAG {


Given that the package is already named dag, I would rename the constructor to New.

Suggested change

func NewDAG() *DAG {

func New() *DAG {

jroimartin · 2025-01-17T12:44:05Z

internal/config/dag/dag.go

+}
+
+// HasVertexBeenAdded checks if a vertex has already been added.
+func (d *DAG) HasVertexBeenAdded(s string) bool {


I would rename this function to Contains. I think it is shorter and easier to understand.

Suggested change

func (d *DAG) HasVertexBeenAdded(s string) bool {

func (d *DAG) Contains(s string) bool {

jroimartin · 2025-01-17T12:48:15Z

internal/engine/engine.go

-	vuln.Description = strings.ReplaceAll(vuln.Description, old, new)
-	vuln.Details = strings.ReplaceAll(vuln.Details, old, new)
-	vuln.ImpactDetails = strings.ReplaceAll(vuln.ImpactDetails, old, new)
+func vulnReplaceAll(vuln report.Vulnerability, oldVuln, newVuln string) report.Vulnerability {


Why this change? Because new is being shadowed? The old naming is common also in the stdlib. For instance,

func ReplaceAll(s, old, new string) string

Reference: https://pkg.go.dev/strings#ReplaceAll

I would honestly silence the linter in this case.

Same goes for rscReplaceAll.

golangci-lint complained about it (https://github.com/adevinta/lava/actions/runs/12828102887/job/35771441550)

jroimartin · 2025-01-17T12:52:36Z

internal/config/resolver.go

+// DAG is the representation of the structure of includes.
+type DAG struct {


I would rename this type to ConfigGraph so it is clear that it is not a generic DAG data structure.

Suggested change

// DAG is the representation of the structure of includes.

type DAG struct {

// ConfigGraph represents the structure of configuration files.

type ConfigGraph struct {

I think it also makes sense to have Config and ConfigGraph. User also don't need to know that it is represented as a DAG internally.

jroimartin · 2025-01-17T12:54:07Z

internal/config/resolver.go

+// NewDAG creates a new DAG that represents the whole configuration.
+func NewDAG(cfg Config, url string) (DAG, error) {


Suggested change

// NewDAG creates a new DAG that represents the whole configuration.

func NewDAG(cfg Config, url string) (DAG, error) {

// NewConfigGraph creates a graph that represents the whole configuration.

func NewConfigGraph(cfg Config, url string) (ConfigGraph, error) {

Also, if we just pass a url here, we avoid the code duplicity of traversing the includes of the provided Config and we can call discoverConfig directly.

jroimartin · 2025-01-17T13:11:11Z

internal/config/resolver.go

I would rename this file to configgraph.go

jroimartin · 2025-01-17T13:13:13Z

internal/config/resolver.go

+	for _, incl := range cfg.Includes {
+		// Skip duplicates.
+		if _, ok := unique[incl]; ok {
+			continue


I would emit a warning saying that a duplicated include has been found.

jroimartin · 2025-01-17T14:48:21Z

internal/engine/engine.go

@@ -312,21 +312,21 @@ func vulnReplaceAll(vuln report.Vulnerability, old, new string) report.Vulnerabi

 // rscReplaceAll returns a copy of the resource group rsc with all
 // non-overlapping instances of old replaced by new.
-func rscReplaceAll(rsc report.ResourcesGroup, old, new string) report.ResourcesGroup {
-	rsc.Name = strings.ReplaceAll(rsc.Name, old, new)
+func rscReplaceAll(rsc report.ResourcesGroup, oldrsc, newrsc string) report.ResourcesGroup {


If you decide to keep the oldVuln/newVuln naming, then I'd rename these ones to oldRsc/newRsc.

seilagamo requested a review from jroimartin September 19, 2024 14:11

internal/config: include remote configuration files

3a1d9cf

seilagamo force-pushed the include-remote-config branch from ec7c67c to 3a1d9cf Compare September 19, 2024 14:27

jroimartin requested changes Jan 14, 2025

View reviewed changes

seilagamo force-pushed the include-remote-config branch 8 times, most recently from 73e5c5e to 332f94d Compare January 17, 2025 11:33

internal/config: fix CR issues

422862c

seilagamo force-pushed the include-remote-config branch from 332f94d to 422862c Compare January 17, 2025 11:37

seilagamo requested a review from jroimartin January 17, 2025 11:44

jroimartin requested changes Jan 17, 2025

View reviewed changes

jroimartin reviewed Jan 17, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

internal/config: include remote configuration files #96

internal/config: include remote configuration files #96

seilagamo commented Sep 19, 2024

jroimartin Jan 17, 2025

seilagamo Jan 17, 2025

jroimartin Jan 17, 2025

jroimartin Jan 17, 2025

jroimartin Jan 17, 2025

jroimartin Jan 17, 2025

jroimartin Jan 17, 2025

seilagamo Jan 17, 2025

jroimartin Jan 17, 2025

jroimartin Jan 17, 2025

jroimartin Jan 17, 2025

jroimartin Jan 17, 2025

jroimartin Jan 17, 2025

jroimartin Jan 17, 2025

		// AddVertex adds the vertex x to the DAG. AddVertex returns an error if s is
		// nil, s is already part of the graph.

	func (d *DAG) HasVertexBeenAdded(s string) bool {
	func (d *DAG) Contains(s string) bool {

		// DAG is the representation of the structure of includes.
		type DAG struct {

		// NewDAG creates a new DAG that represents the whole configuration.
		func NewDAG(cfg Config, url string) (DAG, error) {

internal/config: include remote configuration files #96

Are you sure you want to change the base?

internal/config: include remote configuration files #96

Conversation

seilagamo commented Sep 19, 2024

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