Refactor pkg/config #806

johananl · 2020-08-14T15:46:32Z

This PR is rather cosmetic and focuses on better naming and readability.

The whitespace changes are for satisfying the linter.

invidian

BTW @ipochi did awesome job refactoring pkg/config, which never got trough: https://github.com/kinvolk/lokomotive/pull/359/files#diff-091ace02b49a46f55f2586cfec89a160

pkg/config/config.go

invidian

Also, when we do refactoring, we should also add tests to the code to ensure, that newly introduce code is easily testable.

johananl · 2020-08-25T18:38:25Z

Added some unit tests. They are non-exhaustive but they do demonstrate the testability of the new functions and provides a base for easily adding new tests.

johananl · 2020-08-25T18:39:53Z

TODO: Test lokocfgFiles()?
TODO: Test populateVars()?

invidian

Hm, I still struggle to see the reasoning behind changing the private method names in pkg/config. Perhaps putting a move of the functions inside the file to separate commit would be better, as it would highlight which changes has been performed exactly, maybe I comment code, which didn't change. I'd also split the changes into smaller commits, so then the extra reasoning can be added in the commit messages.

Changes to the exported API seem to indeed make sense.

pkg/config/config.go

pkg/config/config_internal_test.go

pkg/config/config_test.go

johananl · 2020-08-27T19:23:32Z

@invidian I've handled most of your feedback. I still need to edit the commits though.

invidian · 2020-08-28T07:20:00Z

@invidian I've handled most of your feedback. I still need to edit the commits though.

Thanks @johananl! I'll have a look.

invidian

Thanks again for addressing my feedback @johananl, PR looks much better now. I commented on some small nits, but overall it looks nice.

This populateVars still bugs me and now I've noticed, that having RootConfig and Config structs in pkg/config also make little sense and it's related to populateVars. I'll have a look if I can come up with something to improve it, but I think it could be done in separate PR.

pkg/config/config.go

pkg/config/config_internal_test.go

invidian

@johananl I looked at the code around populateVars() and I think I understand now why it was confusing.

I would like to propose following changes on top of yours: 8c76770

They split ReadHCL further, so we can have better named code blocks (as mentioned in the comment) and changes populateVars() into "convert to Map" + "merge maps", which I think is more idiomatic approach than doing both things at the same time, as populateVars() do right now.

pkg/config/config.go

invidian · 2020-08-28T08:34:20Z

pkg/config/config.go

+	names, err := lokocfgFilenames(path)
 	if err != nil {
-		return nil, hcl.Diagnostics{
-			&hcl.Diagnostic{
-				Severity: hcl.DiagError,
-				Summary:  err.Error(),
-			},
-		}
+		return nil, diag(fmt.Sprintf("getting lokocfg paths: %v", err))
 	}

-	hclParser := hclparse.NewParser()
-
-	var hclFiles []*hcl.File
-	for _, f := range lokocfgPaths {
-		hclFile, diags := hclParser.ParseHCLFile(f)
-		if len(diags) > 0 {
-			return nil, diags
-		}
-		hclFiles = append(hclFiles, hclFile)
+	files, err := readFiles(names)
+	if err != nil {
+		return nil, diag(err.Error())
 	}

-	configBody := hcl.MergeFiles(hclFiles)
+	body, diags := parseHCLFiles(files)
+	if diags.HasErrors() {
+		return nil, diags
+	}


To avoid such lengthy functions and to name the blocks of code, I think we could put this three actions into separate function, but I'm not sure how to phrase the name of this function. However, after looking at this code (and code after it), it's clear to me, that this part reads the configuration files (and not values files) and returns them in a form of HCL body.

I don't think ReadHCL() is so long at the moment, and breaking it down further would start to be on the "too much indirection" side. However, looking at the function again I'm not happy with the name ReadHCL since this function does more than just read HCL files. Frankly I think config.New() was the best option from the discussions so far. Do you have an alternative which doesn't include the word "load"? If not, I'd just go with config.New().

I don't think ReadHCL() is so long at the moment, and breaking it down further would start to be on the "too much indirection" side.

Given that this is a exported function, so we should be able to cover it fully with tests, currently has 9 different paths, which IMO is to high cognitive complexity. In 8c76770 I broke it down into nice smaller functions, so we have only 4 paths, which is very reasonable and easy to understand. This is also what tools like https://codeclimate.com/ encourages to do.

I think with the right naming you don't create the indirection, quite the opposite. You hide unrelated details, which makes things simpler and allows understanding only the paths of code which are interesting for the reader. With long function, reader must go trough all of it and understand all the statements to analyze the function.

However, looking at the function again I'm not happy with the name ReadHCL since this function does more than just read HCL files. Frankly I think config.New() was the best option from the discussions so far. Do you have an alternative which doesn't include the word "load"? If not, I'd just go with config.New().

FromHCLFiles() maybe? Or ReadHCLFiles() ? It gives you configuration from HCL files, so I think the name is appropriate. config.New() would be good IMO if we could define the loader type as an argument. Then the name shouldn't imply from where or how do we load the configuration. However, if we take HCL files paths as an argument, name should reflect that too in my opinion.

@johananl I'm currently looking into untangling and isolating code from cli/cmd package, as part of #312 and at the moment I find it confusing, that we call config.LoadConfig(), but it doesn't really load entire configuration. It's even hard to define of what exactly this code is doing. Maybe that will help a bit with naming and documentation here.

This is the definition of existing LoadConfig function I came up with:

// LoadConfig reads and parses top-level HCL configuration from given lokocfgPath and // evaluates the variables from lokocfgVarsPath into HCL context, so it can be used // for futher configuration parsing.

Which leads to the conclusion, that what is being returned is not the configuration (as "user configuration"), but rather a facility, which allows you to further read the configuration.

The lokomotive/pkg/config.Config struct does not ship proper information about itself. It's not "a Lokomotive configuration". It does not represent cluster + components configuration (which we didn't name yet).

The word "packet" in `cluster "packet" {}` is not the cluster name but rather the platform on which the cluster is deployed. Therefore, it makes more sense to name this label "Platform".

- Make function names more concise and accurate. - Reorder functions: - Put methods next to the associated type. - Put exported functions before unexported ones. - Extract some logic from ReadHCL() into smaller functions. - Simplify lokocfgFiles() and rename it to lokocfgFilenames().

invidian · 2020-09-08T07:44:37Z

pkg/config/config.go

-	lokocfgPaths, err := loadLokocfgPaths(lokocfgPath)
+// ReadHCL reads all HCL files at path and one HCL values file at valuesPath, constructs a Config
+// and returns a pointer to it.
+func ReadHCL(path, valuesPath string) (*Config, hcl.Diagnostics) {


I still think this function is too long and should be splitted into smaller ones like I suggested in 8c76770

invidian · 2020-09-08T07:45:35Z

pkg/config/config.go

+// populates each variable with the corresponding user-provided value or with a default. If a
+// variable doesn't have a matching value and there is no default, an error is returned (as
+// hcl.Diagnostics).
+func populateVars(vars []variable, values map[string]cty.Value) (map[string]cty.Value, hcl.Diagnostics) {


@johananl I looked at the code around populateVars() and I think I understand now why it was confusing.

I would like to propose following changes on top of yours: 8c76770

They split ReadHCL further, so we can have better named code blocks (as mentioned in the comment) and changes >populateVars() into "convert to Map" + "merge maps", which I think is more idiomatic approach than doing both things >at the same time, as populateVars() do right now.

johananl requested review from invidian, ipochi and surajssd as code owners August 14, 2020 15:46

invidian suggested changes Aug 14, 2020

View reviewed changes

pkg/config/config.go Show resolved Hide resolved

pkg/config/config.go Outdated Show resolved Hide resolved

pkg/config/config.go Outdated Show resolved Hide resolved

pkg/config/config.go Outdated Show resolved Hide resolved

johananl force-pushed the johananl/refactor-config-parsing branch 3 times, most recently from 2fbe9f3 to 23b9065 Compare August 17, 2020 16:03

invidian suggested changes Aug 18, 2020

View reviewed changes

johananl force-pushed the johananl/refactor-config-parsing branch 3 times, most recently from ca6d670 to 6438a2a Compare August 25, 2020 18:37

johananl force-pushed the johananl/refactor-config-parsing branch from 6438a2a to 462baf9 Compare August 25, 2020 18:42

invidian suggested changes Aug 26, 2020

View reviewed changes

johananl force-pushed the johananl/refactor-config-parsing branch 10 times, most recently from c8784b6 to 98a8aa4 Compare August 27, 2020 19:19

invidian suggested changes Aug 28, 2020

View reviewed changes

pkg/config/config.go Outdated Show resolved Hide resolved

pkg/config/config.go Outdated Show resolved Hide resolved

pkg/config/config.go Outdated Show resolved Hide resolved

pkg/config/config_internal_test.go Outdated Show resolved Hide resolved

invidian suggested changes Aug 28, 2020

View reviewed changes

johananl force-pushed the johananl/refactor-config-parsing branch from 98a8aa4 to 8205933 Compare September 3, 2020 11:38

Rename cluster block label

9d43131

The word "packet" in `cluster "packet" {}` is not the cluster name but rather the platform on which the cluster is deployed. Therefore, it makes more sense to name this label "Platform".

johananl force-pushed the johananl/refactor-config-parsing branch 5 times, most recently from e1a105c to ad5097b Compare September 3, 2020 16:23

johananl removed request for surajssd and ipochi September 3, 2020 16:23

johananl force-pushed the johananl/refactor-config-parsing branch from ad5097b to 2b5b84a Compare September 3, 2020 16:51

johananl requested review from invidian and surajssd September 3, 2020 17:02

invidian suggested changes Sep 8, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor pkg/config #806

Refactor pkg/config #806

johananl commented Aug 14, 2020

invidian left a comment

invidian left a comment

johananl commented Aug 25, 2020

johananl commented Aug 25, 2020 •

edited

Loading

invidian left a comment

johananl commented Aug 27, 2020

invidian commented Aug 28, 2020

invidian left a comment

invidian left a comment

invidian Aug 28, 2020

johananl Sep 3, 2020

invidian Sep 3, 2020

invidian Sep 14, 2020

invidian Sep 14, 2020

invidian Sep 8, 2020

invidian Sep 8, 2020

Refactor pkg/config #806

Are you sure you want to change the base?

Refactor pkg/config #806

Conversation

johananl commented Aug 14, 2020

invidian left a comment

Choose a reason for hiding this comment

invidian left a comment

Choose a reason for hiding this comment

johananl commented Aug 25, 2020

johananl commented Aug 25, 2020 • edited Loading

invidian left a comment

Choose a reason for hiding this comment

johananl commented Aug 27, 2020

invidian commented Aug 28, 2020

invidian left a comment

Choose a reason for hiding this comment

invidian left a comment

Choose a reason for hiding this comment

invidian Aug 28, 2020

Choose a reason for hiding this comment

johananl Sep 3, 2020

Choose a reason for hiding this comment

invidian Sep 3, 2020

Choose a reason for hiding this comment

invidian Sep 14, 2020

Choose a reason for hiding this comment

invidian Sep 14, 2020

Choose a reason for hiding this comment

invidian Sep 8, 2020

Choose a reason for hiding this comment

invidian Sep 8, 2020

Choose a reason for hiding this comment

johananl commented Aug 25, 2020 •

edited

Loading