Save dashboard sha256sum instead of full config json in tfstate

[fgouteroux]

Hi,

In some cases we could have bigger grafana dashboard, that would cause:

• the unability to review changes inside (even in git when we have thousand of lines)
• a bigger statefile size and some errors like Increase Statefile Size  #199

I would like to add the possibility to save only the sha256sum of the config json, if the option config_json_sha256 is set to true.

Also notice that we can go back to the full config json if we disable this parameter by default to false, so there is no breaking change.

[fgouteroux]

no feedback on it ?

[justinTM]

oh this would be huge. thank you. since GitLab jobs have 4MB printout limit, i've just been redirecting the entire Apply to a file because we're at 300k lines of JSON and this would be much better

[inkel]

As far as the changes go it LGTM. Let me discuss it with the rest of the @grafana/terraform-provider team.

[julienduchesne]

Sorry for the delay, I actually thought I had commented this but I never clicked the button 😓

I don't think setting an env variable (the os.Setenv) and using it in the code below is the right approach. Here's what I think should be done:

1. With a DefaultFunc, if GRAFANA_CONFIG_JSON_SHA256 is set to a truthy value (using strconv.ParseBool), then the default is true, otherwise it's false
2. In the code, only use the value of config_json_sha256, at this point, the env variable is irrelevant, it serves as a way to set this globally, but it shouldn't be used in the resources themselves

What this allows is to be able to set this to false and have it true in a single resource or vice versa

[fgouteroux]

Sorry for the delay, I actually thought I had commented this but I never clicked the button 😓

I don't think setting an env variable (the os.Setenv) and using it in the code below is the right approach. Here's what I think should be done:

1. With a DefaultFunc, if GRAFANA_CONFIG_JSON_SHA256 is set to a truthy value (using strconv.ParseBool), then the default is true, otherwise it's false
2. In the code, only use the value of config_json_sha256, at this point, the env variable is irrelevant, it serves as a way to set this globally, but it shouldn't be used in the resources themselves

What this allows is to be able to set this to false and have it true in a single resource or vice versa

@julienduchesne Thanks for the review. I agree with you that setting env var should be not used in the code but this env var is used in the StateFunc: normalizeDashboardConfigJSON of config_json input. I don't know how to get the value of config_json_sha256 input in this StateFunc.

[julienduchesne]

Good point. It doesn't give you the resource data. Then there's no way to handle it on a resource basis so I'd make this a provider setting since it has to be a global setting. Currently, setting the value to true for a single dashboard will enable it for all, this could be confusing

And again, setting a global var on provider init is probably preferable to using os.Getenv at various points (this allows configuration from both a provider option or an env var)

Second thing for this to get merged, is that I'd like a test

[fgouteroux]

I agree, but there is something not clear for me. How to give the provider value 'store_dashboard_sha256' into the state function of the variable 'config_json' in resource_dahboard.go needed for the diff ?

[julienduchesne]

I agree, but there is something not clear for me. How to give the provider value 'store_dashboard_sha256' into the state function of the variable 'config_json' in resource_dahboard.go needed for the diff ?

I'd set it as a global var on provider startup. We don't have many options here unfortunately. At least, it'll be clear that it's global

[fgouteroux]

@julienduchesne I'm not sure that this is what you mean with global var on provider startup. Am I on the good way or not ?

[julienduchesne]

@julienduchesne I'm not sure that this is what you mean with global var on provider startup. Am I on the good way or not ?

Can you make that a regular var: var storeDashboardSHA256 bool in provider.go instead of an env var? That way you won't have to set/parse an envvar

[fgouteroux]

@julienduchesne I'm not sure that this is what you mean with global var on provider startup. Am I on the good way or not ?

Can you make that a regular var: var storeDashboardSHA256 bool in provider.go instead of an env var? That way you won't have to set/parse an envvar

yes, it 's more cleaner...

[julienduchesne]

Alright, this LGTM. Can you add an acceptance test for this?

Just needs a test now

[fgouteroux]

Is that test is enough ?

[julienduchesne]

I think this needs a full acceptance test that uses the actual terraform test framework. This is a major change of behavior of the dashboard resource and we need to check various things like that there isn't drift right after an apply. It should apply the resource and check that the saved state is a sha256, also test an update

Also, I suspect that there is no test that checks that currently, we are saving valid json. We should validate that also in one of the existing tests

[fgouteroux]

@julienduchesne I added the acc test with create/update but it works only with setting env var GRAFANA_STORE_DASHBOARD_SHA256. I try to add this without success

// testAccPreCheckSHA256 verifies required provider testing configuration. It should
// be present in TestAccDashboard_isSHA256_stored acceptance test.
//
// These verifications and configuration are preferred at this level to prevent
// provider developers from experiencing less clear errors for every test.
func testAccPreCheckSHA256(t *testing.T) {
	for _, e := range testAccPreCheckEnv {
		if v := os.Getenv(e); v == "" {
			t.Fatal(e + " must be set for acceptance tests")
		}
	}
	testAccProviderConfigure.Do(func() {
		// Since we are outside the scope of the Terraform configuration we must
		// call Configure() to properly initialize the provider configuration.
		raw := map[string]interface{}{
			"store_dashboard_sha256": true,
		}
		err := testAccProvider.Configure(context.Background(), terraform.NewResourceConfigRaw(raw))
		if err != nil {
			t.Fatal(err)
		}
	})
}

[julienduchesne]

I fixed the tests. This LGTM!

[julienduchesne]

Review without whitespace changes. I reused the basic test but when it's a sha256, the expected state is different

[inkel]

@fgouteroux have you had a chance to look at this comment?

[julienduchesne]

It wasn't a comment for him. Just a general "if you review this test, review it without whitespace". I wrote that test 😄

[inkel]

Aaaahhh ok, my bad, then.

So, just to see if I got this straight: the test fails?

[julienduchesne]

No, the test passes. It just shows lots of changes because indentation changes

[fgouteroux]

I fixed the tests. This LGTM!

@julienduchesne many thanks

[inkel]

I'm reviewing, give me a couple minutes.

[inkel]

I've a minor request, once that is addressed I'll be happy to re-review and approve.

[inkel]

Great work, @fgouteroux! Thank you for your contribution.

[fgouteroux]

Great work, @fgouteroux! Thank you for your contribution.

@inkel @julienduchesne Thanks for your tips/feedback. I'm happy to see it merged ;-)