hashicorp
TerraformConfig: Add KeyValues method
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.
@wilkermichael Sorry for the late response, was going through a lot of study work, and now I am looking at this.
Hey @wilkermichael I have a few questions
-
What can be done here? Should this field be omitted in json using
json:"-"https://github.com/hashicorp/consul-terraform-sync/blob/cf630a15e69a6342fe44fd7434283022b36b457c/config/condition_catalog_services.go#L12-L14 -
Some structs are implementing this interface which has
GoStringso I cannot renameGoStringmethod toStringof these structs, should we rename the method inMonitorConfiginterface too? https://github.com/hashicorp/consul-terraform-sync/blob/c0c4728b7ded880bc5ad47e951a79b81fd5cb6d3/config/monitor.go#L9-L15
Hey @wilkermichael I have a few questions
- What can be done here? Should this field be omitted in json using
json:"-"https://github.com/hashicorp/consul-terraform-sync/blob/cf630a15e69a6342fe44fd7434283022b36b457c/config/condition_catalog_services.go#L12-L14- Some structs are implementing this interface which has
GoStringso I cannot renameGoStringmethod toStringof these structs, should we rename the method inMonitorConfiginterface too? https://github.com/hashicorp/consul-terraform-sync/blob/c0c4728b7ded880bc5ad47e951a79b81fd5cb6d3/config/monitor.go#L9-L15
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!
@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.
@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 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.
@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.
@thisisommore Happy New Year!
No rush on this, take your time. I probably won't get a chance to review until next week.
@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.
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