Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support testing of configurations in JSON syntax. #722

Merged
merged 3 commits into from
Mar 11, 2022

Conversation

rudo-thomas
Copy link
Contributor

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
Copy link

hashicorp-cla commented Mar 9, 2021

CLA assistant check

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.

Base automatically changed from master to main March 22, 2021 14:01
@bflad bflad added enhancement New feature or request subsystem/tests Issues and feature requests related to the testing framework. labels Oct 12, 2021
@bflad
Copy link
Contributor

bflad commented Oct 12, 2021

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.)

@bflad bflad added the waiting-response Issues or pull requests waiting for an external response label Oct 12, 2021
rudo-thomas added a commit to rudo-thomas/terraform-plugin-sdk that referenced this pull request Jan 7, 2022
@rudo-thomas
Copy link
Contributor Author

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 bflad removed the waiting-response Issues or pull requests waiting for an external response label Jan 11, 2022
@bflad bflad added this to the v2.12.0 milestone Mar 10, 2022
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines -145 to -178
func (wd *WorkingDir) configFilename() string {
return filepath.Join(wd.baseDir, ConfigFileName)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

@@ -135,17 +153,13 @@ func (wd *WorkingDir) ClearPlan() error {
// Init runs "terraform init" for the given working directory, forcing Terraform
// to use the current version of the plugin under test.
func (wd *WorkingDir) Init() error {
if _, err := os.Stat(wd.configFilename()); err != nil {
if wd.configFilename == "" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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().

Comment on lines 92 to 97
if wd.configFilename != "" {
err := os.Remove(wd.configFilename)
if err != nil && !os.IsNotExist(err) {
return err
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
	}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

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

Thanks for review, @bflad
PTAL

rudo-thomas added a commit to rudo-thomas/terraform-plugin-sdk that referenced this pull request Mar 11, 2022
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@bflad bflad requested a review from a team as a code owner March 11, 2022 21:16
@bflad bflad self-assigned this Mar 11, 2022
@bflad bflad linked an issue Mar 11, 2022 that may be closed by this pull request
@bflad bflad merged commit aa11e4c into hashicorp:main Mar 11, 2022
bflad added a commit that referenced this pull request Mar 11, 2022
@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request subsystem/tests Issues and feature requests related to the testing framework.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support configurations in JSON syntax in the provider test framework
3 participants