TerraformConfig: Add KeyValues method #506

thisisommore · 2021-11-13T04:55:57Z

Work in progress PR to fix #377

hashicorp-cla · 2021-11-13T04:56:00Z

All committers have signed the CLA.

thisisommore · 2021-11-13T04:59:26Z

@wilkermichael Is this the way we should go over all the structs?

wilkermichael · 2021-11-17T17:44:30Z

Hey @thisisommore, thanks for taking a look at this! This solution definitely is an improvement towards querying and computer parsing.

After doing some investigation myself, I realised that the log library we’re using supports JSON output which would be the ideal format for querying tools. Therefore, it makes more sense to make sure the JSON is queryable, and the focus of the human-readable output should be on clarity. For example, I've written a test case that demos what that might look like with your change:

func TestInterface(t *testing.T) {
	tc := TerraformConfig{
		Version:    String("test"),
		Log:        Bool(true),
		WorkingDir: String("test"),
	}
	var cc ConsulConfig
	cc.Finalize()
	tc.Finalize(&cc)

        // Output human readable
	logging.Global().Info("test", tc.KeyValues()...)

	logger := hclog.New(&hclog.LoggerOptions{
		JSONFormat: true, // in the future support a flag to toggle this JSON format
		TimeFormat: hclog.TimeFormat,
	})
	hclog.SetDefault(logger)
        
        // Output JSON
	logging.Global().Info("test", tc.KeyValues()...)
}

Which outputs the human readable:

2021-11-16T11:58:24.640-0800 [INFO] test: Version=test Log=true PersistLog=false Path=/Users/mwilkerson/dev/consul-terraform-sync/config WorkingDir=test Backend="map[consul:map[address:localhost:8500 gzip:true path:consul-terraform-sync/terraform]]" RequiredProviders=map[]

and the json

{
   "@level":"info",
   "@message":"test",
   "@timestamp":"2021-11-16T11:49:13.914-0800",
   "Backend":{
      "consul":{
         "address":"localhost:8500",
         "gzip":true,
         "path":"consul-terraform-sync/terraform"
      }
   },
   "Log":true,
   "Path":"/Users/mwilkerson/dev/consul-terraform-sync/config",
   "PersistLog":false,
   "RequiredProviders":{
      
   },
   "Version":"test",
   "WorkingDir":"test"
}

One limitation with the solution here is that the JSON object doesn't include any reference to the original object, in this case TerraformConfig, so a user wouldn't be able to query for example terraformConfig.Version="test".

After investigating a bit more, I’ve added a possible alternative solution, more information and examples can now be found in the issue details. Let me know how you feel about them. I know it's a lot of information at once, so don’t hesitate to reach out if you have more questions.

thisisommore · 2021-12-13T08:30:38Z

@wilkermichael Sorry for the late response, was going through a lot of study work, and now I am looking at this.

thisisommore · 2021-12-16T04:30:04Z

Hey @wilkermichael
I have a few questions

What can be done here?
Should this field be omitted in json using json:"-"

consul-terraform-sync/config/condition_catalog_services.go

Lines 12 to 14 in cf630a1

    
           type CatalogServicesConditionConfig struct { 
        
           	CatalogServicesMonitorConfig `mapstructure:",squash"` 
        
           }

Some structs are implementing this interface which has GoString so I cannot rename GoString method to String of these structs, should we rename the method in MonitorConfig interface too?

consul-terraform-sync/config/monitor.go

Lines 9 to 15 in c0c4728

    
           type MonitorConfig interface { 
        
           	Copy() MonitorConfig 
        
           	Merge(MonitorConfig) MonitorConfig 
        
           	Finalize([]string) 
        
           	Validate() error 
        
           	GoString() string 
        
           }

wilkermichael · 2021-12-20T19:48:34Z

Hey @wilkermichael I have a few questions

What can be done here?
Should this field be omitted in json using json:"-"

consul-terraform-sync/config/condition_catalog_services.go

Lines 12 to 14 in cf630a1

type CatalogServicesConditionConfig struct {

CatalogServicesMonitorConfig `mapstructure:",squash"`

}

Some structs are implementing this interface which has GoString so I cannot rename GoString method to String of these structs, should we rename the method in MonitorConfig interface too?

consul-terraform-sync/config/monitor.go

Lines 9 to 15 in c0c4728

type MonitorConfig interface {

Copy() MonitorConfig

Merge(MonitorConfig) MonitorConfig

Finalize([]string)

Validate() error

GoString() string

}

Hey @thisisommore, thanks for your patience while I got back to you. I'll address each question here:

(1) I just did a quick test to verify the current default behaviour of that squashed field when logging json:

c := &CatalogServicesConditionConfig{
				CatalogServicesMonitorConfig{
					Regexp:            String(".*"),
					SourceIncludesVar: Bool(true),
					Datacenter:        String("dc2"),
					Namespace:         String("ns2"),
					NodeMeta: map[string]string{
						"key1": "value1",
						"key2": "value2",
					},
				},
			}

// JSON logging enabled
logging.Global().Info("this is a json log", "catalogServicesConditionConfig", c)`

yields the following log:

{
    "@level": "info",
    "@message": "this is a json log",
    "@timestamp": "2021-12-20T11:31:26.262-0800",
    "catalogServicesConditionConfig": {
        "Regexp": ".*",
        "SourceIncludesVar": true,
        "Datacenter": "dc2",
        "Namespace": "ns2",
        "NodeMeta": {
            "key1": "value1",
            "key2": "value2"
        }
    }
}

Ignoring the field names since I didn't setup struct tags for them, the general structure of this json makes sense to me. It looks like it doesn't include the squashed field name in the JSON which I think is good. If you add the json:"-" tag I believe the struct won't be included at all. I therefore suggest you not add json tags to squashed fields like in the example you provided.

(2) I think renaming the interface method to String() makes sense!

thisisommore · 2021-12-22T09:37:51Z

@wilkermichael Thanks for addressing this, I have committed the changes, and now will try creating tests.
You can review my changes and let me know if any change is needed.

…ruct-key-values-377

thisisommore · 2022-01-02T15:48:48Z

@wilkermichael do I have a write the test for each struct?

wilkermichael · 2022-01-06T22:20:04Z

@wilkermichael do I have a write the test for each struct?

@thisisommore thanks for your patience, I've been on holidays the last couple of weeks. I think it would be a good idea to have a test for each struct, however, you can just test whether or not the object marshal's successfully. You don't need to check that the string matches for each case.

wilkermichael · 2022-01-18T22:51:04Z

@wilkermichael Thanks for addressing this, I have committed the changes, and now will try creating tests. You can review my changes and let me know if any change is needed.

@thisisommore I was looking through again and noticed you asked me to do a preliminary review. I'm happy to do one for you, however, can you please rebase your changes onto main first and resolve the conflicts? There have been a number of changes to the config since work was started on this PR.

…ruct-key-values-377

thisisommore · 2022-01-20T15:53:31Z

@thisisommore I was looking through again and noticed you asked me to do a preliminary review. I'm happy to do one for you, however, can you please rebase your changes onto main first and resolve the conflicts? There have been a number of changes to the config since work was started on this PR.

@wilkermichael sorry for being late, I have merged main into this PR, also I was not going into the test due to now sure where to start, I hope this is not urgent or feel free to assign this to someone else, I will be trying hands on it again this Sunday.

Also Happy New Year 🥳, hope you are doing well.

wilkermichael · 2022-01-21T16:24:58Z

@thisisommore Happy New Year!

No rush on this, take your time. I probably won't get a chance to review until next week.

wilkermichael

Hey @thisisommore I have reviewed some of your submission, but noticed that there are some build issues as well as some tests which I don't think will pass. Can you please resolve all build issues and confirm the unit tests are passing?

You can run the following to verify this behaviour:

make dev will attempt to build CTS
make test will run the unit tests

Thanks for all your hard work, an I will continue the review after these are resolved

wilkermichael · 2022-01-27T22:53:33Z

command/cli.go

@@ -181,7 +181,7 @@ func (cli *CLI) runBinary(configFiles, inspectTasks config.FlagAppendSliceValue,

 	// Print information on startup for debugging
 	logger.Info(version.GetHumanVersion())
-	logger.Debug("configuration", "config", conf.GoString())
+	logger.Debug("configuration", "config", conf.String())


The cool thing about the Stringer interface is that the logger will actually try and call the interface without the implementer needing to explicitly call the String() method. In this way the logger can marshal to JSON or call String() when printing plaintext.

Can you remove the String() call from instances where the logger calls the String() method?

eg. in this case logger.Debug("configuration", "config", conf)

wilkermichael · 2022-01-27T22:55:13Z

config/auth.go

-	Password *string `mapstructure:"password"`
+	Enabled  *bool   `mapstructure:"enabled" json"enabled"`
+	Username *string `mapstructure:"username" json"username"`
+	Password *string `mapstructure:"password" json"password"`
 }


I think you need a : between json and the field name
i.e. json:"enabled"

can you also use json:"-" for the password string, this shouldn't be included when printing the json

I'll try to catch similar scenarios where you should use json:"-" but if you see it's printing the value using the sensitiveGoString() function in the String() method, then the json tag should be redacted as well.

wilkermichael · 2022-01-28T19:39:45Z

config/condition_catalog_services.go

 		"%s"+
 		"}",
-		c.CatalogServicesMonitorConfig.GoString(),
+		c.CatalogServicesMonitorConfig.String(),


Can remove .String() usage here as well, and all other usages like this

wilkermichael · 2022-01-28T19:41:03Z

config/condition_consul_kv.go

 	return fmt.Sprintf("&ConsulKVConditionConfig{"+
 		"%s, "+
 		"UseAsModuleInput:%v"+
 		"}",
 		c.ConsulKVMonitorConfig.GoString(),
 		BoolVal(c.UseAsModuleInput),
+>>>>>>> upstream/main


Please fix this merge conflict

wilkermichael · 2022-01-28T19:43:39Z

config/condition_schedule.go


-	return fmt.Sprintf("&ScheduleConditionConfig{"+
+	return fmt.Sprintf("{"+
 		"Cron:%s, "+


Missing json tag for Cron (json:"cron") on above ScheduleConditionConfig struct (line: 15)

wilkermichael · 2022-01-28T19:47:48Z

config/condition_services.go


 	return fmt.Sprintf("&ServicesConditionConfig{"+
 		"%s, "+
 		"UseAsModuleInput:%v"+
 		"}",
-		c.ServicesMonitorConfig.GoString(),
+		c.ServicesMonitorConfig.String(),
 		BoolVal(c.UseAsModuleInput),
 	)
 }


Missing json tags on ServicesConditionConfig struct. Most likely these were added after you merged main

wilkermichael · 2022-01-28T19:53:33Z

config/consul.go

 		sensitiveGoString(c.Token),
-		c.Transport.GoString(),
+		c.Transport.String(),
 	)
 }



Missing json struct tags on ConsulConfig

There is a special case here, notice how c.Token is "sensitive" in this case I think it would be appropriate to use the special json tag json:"-" so as to not include this field when printing out

wilkermichael · 2022-01-28T21:19:21Z

@thisisommore I changed some settings on our end as I noticed that the pipeline wasn't running for your PR. The checks should now show the areas where the PR is failing.

wilkermichael · 2022-01-28T21:29:26Z

You can ignore the Slack failure, that one is on our end. It requires a setting change, but won't affect your PR.

EDIT: This has been resolved now in the latest main, if you rebase on the latest main it should resolve the slack failure in the pipeline

TerraformConfig: Add KeyValues method

6c26113

Rename method GoString to String and add json struct tags

898f90a

thisisommore added 2 commits January 2, 2022 15:29

replace GoString method call with String call in remaining places

b4cbe07

Merge remote-tracking branch 'upstream/main' into thisisommore/log-st…

b17e435

…ruct-key-values-377

thisisommore added 2 commits January 20, 2022 15:35

fix errors

c639ec0

Merge remote-tracking branch 'upstream/main' into thisisommore/log-st…

00ea382

…ruct-key-values-377

thisisommore marked this pull request as ready for review January 20, 2022 15:49

thisisommore requested review from a team and lornasong and removed request for a team January 20, 2022 15:49

lornasong requested review from wilkermichael and removed request for lornasong January 20, 2022 17:20

wilkermichael reviewed Jan 28, 2022

View reviewed changes

wilkermichael force-pushed the thisisommore/log-struct-key-values-377 branch from 3437cd4 to 00ea382 Compare January 28, 2022 21:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TerraformConfig: Add KeyValues method #506

TerraformConfig: Add KeyValues method #506

thisisommore commented Nov 13, 2021

hashicorp-cla commented Nov 13, 2021 •

edited

Loading

thisisommore commented Nov 13, 2021

wilkermichael commented Nov 17, 2021

thisisommore commented Dec 13, 2021

thisisommore commented Dec 16, 2021 •

edited

Loading

wilkermichael commented Dec 20, 2021 •

edited

Loading

thisisommore commented Dec 22, 2021

thisisommore commented Jan 2, 2022

wilkermichael commented Jan 6, 2022

wilkermichael commented Jan 18, 2022

thisisommore commented Jan 20, 2022 •

edited

Loading

wilkermichael commented Jan 21, 2022

wilkermichael left a comment •

edited

Loading

wilkermichael Jan 27, 2022

wilkermichael Jan 27, 2022

wilkermichael Jan 28, 2022

wilkermichael Jan 28, 2022

wilkermichael Jan 28, 2022

wilkermichael Jan 28, 2022

wilkermichael Jan 28, 2022

wilkermichael Jan 28, 2022

wilkermichael Jan 28, 2022

wilkermichael Jan 28, 2022

wilkermichael commented Jan 28, 2022

wilkermichael commented Jan 28, 2022 •

edited

Loading

TerraformConfig: Add KeyValues method #506

Are you sure you want to change the base?

TerraformConfig: Add KeyValues method #506

Conversation

thisisommore commented Nov 13, 2021

hashicorp-cla commented Nov 13, 2021 • edited Loading

thisisommore commented Nov 13, 2021

wilkermichael commented Nov 17, 2021

thisisommore commented Dec 13, 2021

thisisommore commented Dec 16, 2021 • edited Loading

wilkermichael commented Dec 20, 2021 • edited Loading

thisisommore commented Dec 22, 2021

thisisommore commented Jan 2, 2022

wilkermichael commented Jan 6, 2022

wilkermichael commented Jan 18, 2022

thisisommore commented Jan 20, 2022 • edited Loading

wilkermichael commented Jan 21, 2022

wilkermichael left a comment • edited Loading

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

wilkermichael commented Jan 28, 2022

wilkermichael commented Jan 28, 2022 • edited Loading

hashicorp-cla commented Nov 13, 2021 •

edited

Loading

thisisommore commented Dec 16, 2021 •

edited

Loading

wilkermichael commented Dec 20, 2021 •

edited

Loading

thisisommore commented Jan 20, 2022 •

edited

Loading

wilkermichael left a comment •

edited

Loading

wilkermichael commented Jan 28, 2022 •

edited

Loading