Support testing of configurations in JSON syntax.

[rudo-thomas]

This implements the proposal described in #721

The exported plugintest.WorkingDir.SetConfig API was deliberately left unmodified to avoid a breaking change.

The only possible breaking change is the mid-struct addition of the ConfigIsJSON field to TestStep which could break users that initialise the structure without using the the Field: syntax. Such uses are very unlikely, as the structure had 19 fields already, 20 with this change.

I wasn't able to find a good place to add tests for the new functionality. I'll be happy to take any advice on this. Thanks.

[hashicorp-cla]

Thank you for your submission! We require that all contributors sign our Contributor License Agreement ("CLA") before we can accept the contribution. Read and sign the agreement

Learn more about why HashiCorp requires a CLA and what the CLA includes

---

0 out of 2 committers have signed the CLA.

• bflad
• rudo-thomas

_{Have you signed the CLA already but the status is still pending? Recheck it.}

[bflad]

Hi @rudo-thomas 👋 Thank you for raising this, it is a nice enhancement.

I'm curious if instead of requiring an additional and explicit configuration option, if this could instead be automatically determined by the content being passed through Config already -- e.g. just checking if the string parses as JSON. This would save threading the new option all the way through and could save some developer trouble. What do you think?

(Aside: Please note that the maintainers handle updating the CHANGELOG.md in this project on merge.)

[rudo-thomas]

Thanks @bflad , your suggestion makes perfect sense: Automatic detection is much easier to use, there is no ambiguity (a HCL config cannot be valid JSON and vice-versa), and the implementation is cleaner too :)

I've re-worked and re-based this PR.

PTAL

[bflad]

Overall this is looking pretty good! Just some minor refactoring and I think this will be good to go.

[bflad]

For consistency, I think we should still keep this method around to automatically do the file joining:

func (wd *WorkingDir) configFilename() string {
	return filepath.Join(wd.baseDir, wd.configFilename)
}

And set without baseDir via:

if json.Valid(bCfg) {
	wd.configFilename = ConfigFileNameJSON
} else {
	wd.configFilename = ConfigFileName
}

[rudo-thomas]

Unlike WorkingDir.planFilename() -- which is "static" in the sense that it only depends on the Workdir.baseDir, configFilename is now "dynamic". It can be empty (SetConfig() not called yet) or one of the two cases (HCL/JSON).

When I tried your suggestion, configFilename() would:

1. either have to return (string, error) (error in case no filename has been set up yet), or the caller would be responsible,
2. or the caller would be responsible to check the filename field for emptiness.

The first option is of course the clean and correct one, but it felt cumbersome at the only place where this would be called (Init(), see below).

If you feel strongly, I can refactor it to the method that returns (string, error).

[bflad]

The original os.Stat() is likely safer here, in case configFilename isn't reset to "". 👍

[rudo-thomas]

Based on the error message, I feel like the original os.Stat() is here as a test whether SetConfig has ever been called.

I re-added the call to os.Stat().

[bflad]

Rather than checking for the filename, we should unilaterally remove/overwrite both the HCL and JSON configuration files here, just to prevent any potential oddities. If we want to be slightly more efficient, it can be done based on the json.Valid() conditional below 👍

if json.Valid(bCfg) {
	wd.configFilename = ConfigFileNameJSON

	if err := os.Remove(filepath.Join(wd.baseDir, ConfigFileName)); err != nil && !errors.Is(err, fs.ErrNotExist) {
		return fmt.Errorf("unable to remove %q: %w", ConfigFileName, err)
	}
} else {
	wd.configFilename = ConfigFileName

	if err := os.Remove(filepath.Join(wd.baseDir, ConfigFileNameJSON)); err != nil && !errors.Is(err, fs.ErrNotExist) {
		return fmt.Errorf("unable to remove %q: %w", ConfigFileNameJSON, err)
	}
}

[rudo-thomas]

Done.

I have also added a test that all of the deleting works.

Thanks for review, @bflad
PTAL

[bflad]

Looks good to me, thank you for the updates and tests, @rudo-thomas 🚀

[github-actions]

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.