Update validations

[uturunku1]

Description

This PR introduces validation check for all "include" values. Previously we had 3 files that had helper functions to check that the values passed as "filter" option and "include" option were valid strings. This release seemed like a good opportunity to standardize this practice beyond those 3 files.

Also, as we have recently made all "include" options a typed string field, it makes validation of those values more convenient.

Keep in mind that throwing errors for invalid include params is a nice bonus that we are adding to go-tfe and not a required one because this behavior does not exactly align with the response you'd get from the platform. To understand this point with a better context, read the comment I left for one of those validations

go-tfe/admin_run.go

Lines 134 to 147 in 76df95c

	func validateAdminRunFilterParams(runStatus string) error {
	// For the platform, an invalid filter value is a semantically understood query that returns an empty set, no error, no warning. But for go-tfe, an invalid value is good enough reason to error prior to a network call to the platform:
	if validString(&runStatus) {
	runStatuses := strings.Split(runStatus, ",")
	// iterate over our statuses, and ensure it is valid.
	for _, status := range runStatuses {
	switch status {
	case string(RunApplied), string(RunApplyQueued), string(RunApplying), string(RunCanceled), string(RunConfirmed), string(RunCostEstimate), string(RunCostEstimating), string(RunDiscarded), string(RunErrored), string(RunPending), string(RunPlanQueued), string(RunPlanned), string(RunPlannedAndFinished), string(RunPlanning), string(RunPolicyChecked), string(RunPolicyChecking), string(RunPolicyOverride), string(RunPolicySoftFailed):
	// do nothing
	default:
	return fmt.Errorf(`invalid value "%s" for run status`, status)
	}
	}
	}

Implementation Details

I added

if err := options.valid(); err != nil {
		return nil, err
	}

to listing and reading functions that take in options with Include values.

Within the valid() func, I call a helper function that validates that those include values are the listed as constants IncludeOpt typed string

if err := validateAdminOrgIncludeParams(o.Include); err != nil {
		return err
	}

I added some tests to validate this new functionality.

Important note

A change not directly related to this PR, but that the new functionality helped surface as an issue, is that a previous change I had introduced on PR #339 was invalid. So I reverted that change in this PR. You can find that change in the following files:

agent_pool.go
agent_pool_integration_test.go

[brandonc]

Minor issue detected?

[brandonc]

Is it not possible to define RunStatus as type RunStatus on AdminRunsListOptions so you can avoid the string type here?

[uturunku1]

Indeed! I'll make it into a typed string.

[uturunku1]

On second thought, converting RunStatus filter option from a string into a typed string lis going to a larger deal than expected. Filter options is a comma-separated list https://www.terraform.io/cloud-docs/api-docs/admin/runs#query-parameters just like Include options. So I’d have to apply the same types of changes I made for include field: make it into a slice of typed strings, then convert it back into string of comma-separated values when I call client.newRequest . Then I would have to apply the same changes to all filter options not just RunStatus to create consistency in this behavior for all filter options :(

[brandonc]

Looks good, but I wonder if we are being too strict and not creating a situation that can't be worked around if the client lags behind the API in some of these cases

[brandonc]

I wonder if we should allow an empty string because one could be split if there was a trailing comma. The API would accept a trailing comma, I think.

[uturunku1]

Good call! It could also have trailing space. OK! I am writing a new test for this.

[brandonc]

💅 I don't care one way or the other but this might be easier to read if it were in multiple lines

[uturunku1]

Yes

[uturunku1]

This test and all the ones for TestAdminRuns_AdminRunsListOptions_valid don't really need to be part of integration test suite. We are just testing helper functions and it'd be nice if external contributors could have access to running these tests. I'd create a unit test file, like I did with tfe_test.go, but then suddenly I end up with 2 new unit test files, suddenly I feel like they should have their own directory to live..., suddenly I have the urge to reorganize other files. But this is not the time to reorg things.
The subject of reorganizing the structure of this project for upcoming 1.0 release has been discussed (because we know is not scalable to continue adding new files to root directory). We decided to postpone. It would involve too many breaking changes on top of the ones we are already bringing. But I really hope that we can find a solution to the "how to continue adding files to this project" topic.