Add policy set version

[rberlind]

Description

This PR adds support for policy set versions. This was requested by a significant HashiCorp customer who wants to be able to use the TFE provider to manage non-VCS-backed policy sets for which they currently have to use the TFE API itself to create policy set versions and then upload their actual policy sets to the upload URL of the policy set versions. After this PR is approved, I plan on doing one against the TFE provider itself that will use the new policy_set_version.go class that I have added in this PR.

I encountered some complications in adding this resource because the upload link for policy set versions is returned under the links collection of the JSON document returned by the API that creates policy set versions. Unfortunately, the jsonapi package that the go-tfe library uses does not support unmarshalling those links with structure tags the way it supports unmarshalling relationships. See https://github.com/google/jsonapi/blob/master/README.md#links, google/jsonapi#93, and google/jsonapi#68. While the last proposes a solution and references a working example in https://github.com/google/jsonapi/blob/master/request_test.go#L694, that example does not actually include demarshalling a structure that uses jsonapi structure tags. And the JSONAPILinks example given in https://github.com/google/jsonapi/blob/master/README.md#links would appear to only help with marshalling links into a structure.

I addressed this problem in the following ways:

1. I used json structure tags instead of jsonapi structure tags in policy_set_version.go
2. I added special processing to the do() function in tfe.go to handle structures that do not have any jsonapi structure tags. When that is the case, instead of returning jsonapi.UnmarshalPayload(resp.Body, v), the function returns json.NewDecoder(resp.Body).Decode(v).
3. To support that special processing, I also added logic to the do() method of tfe.go similar to that in the serializeRequestBody() function of tfe.go to determine whether v is a Slice or a Ptr and count the number of jsonapi and json fields in v.

Since all other response structures currently used have at least one jsonapi structure tag, only policy set versions are affected by the changes in items 2 and 3.

I also added a policy set with one policy set definition file and one policy under test-fixutres/policy-set-version. This is used by the TestPolicySetVersionsUpload() function of policy_set_version_test.go and by main.go of examples/policysets.

I added a new example to create policy sets and policy set versions under examples/policysets. Note that this reads the TFE_ADDRESS, TFE_TOKEN, and TFE_ORGANIZATION environment variables, avoiding the need to edit the file to set hard-coded files. Similar changes could be made to examples/organizations/main.go and examples/workspaces/main.go, but I am not making them in this PR.

Testing plan

You can test the new code in two ways:

1. Export environment variables described in TESTS.md and then run go test -run TestPolicySetVersion -v ./...
2. Run go install . from the root directory of go-tfe, navigate to the examples/policysets directory, run go install . again, export the TFE_ADDRESS, TFE_TOKEN, and TFE_ORGANIZATION environment variables, and then run $GOPATH/bin/policysets.

External links

• jsonapi package
• jsonapi Links
• jsonapi issue 93
• jsonapi issue 68

Output from tests

Here are the results of testing against policy_set_version_test.go:

$go test -run TestPolicySetVersion -v ./...                               
=== RUN   TestPolicySetVersionsCreate
=== RUN   TestPolicySetVersionsCreate/with_valid_options
=== RUN   TestPolicySetVersionsCreate/when_policy_set_ID_is_invalid
--- PASS: TestPolicySetVersionsCreate (0.89s)
    --- PASS: TestPolicySetVersionsCreate/with_valid_options (0.17s)
    --- PASS: TestPolicySetVersionsCreate/when_policy_set_ID_is_invalid (0.00s)
=== RUN   TestPolicySetVersionsRead
=== RUN   TestPolicySetVersionsRead/when_the_policy_set_version_exists
=== RUN   TestPolicySetVersionsRead/when_the_policy_set_version_does_not_exist
=== RUN   TestPolicySetVersionsRead/with_invalid_policy_set_version_id
--- PASS: TestPolicySetVersionsRead (0.71s)
    --- PASS: TestPolicySetVersionsRead/when_the_policy_set_version_exists (0.06s)
    --- PASS: TestPolicySetVersionsRead/when_the_policy_set_version_does_not_exist (0.04s)
    --- PASS: TestPolicySetVersionsRead/with_invalid_policy_set_version_id (0.00s)
=== RUN   TestPolicySetVersionsUpload
=== RUN   TestPolicySetVersionsUpload/with_valid_options
=== RUN   TestPolicySetVersionsUpload/without_a_valid_upload_URL
=== RUN   TestPolicySetVersionsUpload/without_a_valid_path
--- PASS: TestPolicySetVersionsUpload (2.01s)
    --- PASS: TestPolicySetVersionsUpload/with_valid_options (1.27s)
    --- PASS: TestPolicySetVersionsUpload/without_a_valid_upload_URL (0.02s)
    --- PASS: TestPolicySetVersionsUpload/without_a_valid_path (0.00s)
PASS
ok  	github.com/hashicorp/go-tfe	3.782s
?   	github.com/hashicorp/go-tfe/examples/organizations	[no test files]
?   	github.com/hashicorp/go-tfe/examples/policysets	[no test files]
?   	github.com/hashicorp/go-tfe/examples/workspaces	[no test files]

Here are results from running $GOPATH/bin/policysets from examples/policysets:

$$GOPATH/bin/policysets
2020/11/09 10:35:46 The policy set ID is:polset-vqzQART8KsoW3Yjq
2020/11/09 10:35:46 The policy set version Type is: policy-set-versions
2020/11/09 10:35:46 The policy set version ID is: polsetver-yjV8gjDNAZm7JJyN
2020/11/09 10:35:46 The linked policy set ID is: polset-vqzQART8KsoW3Yjq
2020/11/09 10:35:46 The upload link is:https://roger-ptfe.hashidemos.io/_archivist/v1/object/dmF1bHQ6djE6bHoyN2RUTFFSVEFSOFhUT0l1UElQUmZLN20wTENxWVRIcUdNMXRxNzhKTDJnWCtPcjVhWFdTRS8wSE9jdTYxWTArejlXOEhXY1J1bWVkclp6bUdwVW1wTkRXUEJyU0VpMVllZ1ozVHQ5cmgveFhMS25BT09vcC9wcy9MVWRCQjI1SXcrZUNtUmdQRU9Oc0JsWm9MN2ZqSmhpU2FMbng0UWI2NU1nYloreTBiclJpdmlJMzJYckNmU2NwRXNIQVB6VGs4WFMwWWY5NS9vYjlJcUZXUklpa0F6alRlZTl3bkd4b1VDRnpqSElhQkZvWE5tNmFYaFh5NXpGM095ckFockk3d2w2TldWWHRaMzFTdXE5b0xJUzkrUA
2020/11/09 10:35:46 The upload status is: pending
2020/11/09 10:35:46 The upload URL is:https://roger-ptfe.hashidemos.io/_archivist/v1/object/dmF1bHQ6djE6bHoyN2RUTFFSVEFSOFhUT0l1UElQUmZLN20wTENxWVRIcUdNMXRxNzhKTDJnWCtPcjVhWFdTRS8wSE9jdTYxWTArejlXOEhXY1J1bWVkclp6bUdwVW1wTkRXUEJyU0VpMVllZ1ozVHQ5cmgveFhMS25BT09vcC9wcy9MVWRCQjI1SXcrZUNtUmdQRU9Oc0JsWm9MN2ZqSmhpU2FMbng0UWI2NU1nYloreTBiclJpdmlJMzJYckNmU2NwRXNIQVB6VGs4WFMwWWY5NS9vYjlJcUZXUklpa0F6alRlZTl3bkd4b1VDRnpqSElhQkZvWE5tNmFYaFh5NXpGM095ckFockk3d2w2TldWWHRaMzFTdXE5b0xJUzkrUA
2020/11/09 10:35:46 The upload status is: ready
2020/11/09 10:35:46 Successfully deleted policy set polset-vqzQART8KsoW3Yjq

I also ran the complete test suite with go test -v -count=1 ./... -timeout=30m > testresult.txt and have attached output in which all tests did pass:
testresults-passed.txt

However, I also noticed that some unrelated tests sometimes failed, giving errors when deleting workspaces. Those workspaces ultimately disappeared, but I believe because the organizations containing them were deleted. I reduced the frequency of these failures by adding a 1 second sleep at the beginning of the orgCleanup() function returned by the createWorkspace() function of helper_test.go. This seems to help by giving TFE a bit more time after previous operations done against workspaces before deleting them and does not make the testing take much longer.

Here is an example of the type of error I had been seeing:

=== RUN   TestRunsCancel
=== RUN   TestRunsCancel/when_the_run_exists
=== RUN   TestRunsCancel/when_the_run_does_not_exist
=== RUN   TestRunsCancel/with_invalid_run_ID
--- FAIL: TestRunsCancel (2.25s)
    --- PASS: TestRunsCancel/when_the_run_exists (0.04s)
    --- PASS: TestRunsCancel/when_the_run_does_not_exist (0.03s)
    --- PASS: TestRunsCancel/with_invalid_run_ID (0.00s)
    helper_test.go:914: Error destroying workspace! WARNING: Dangling resources
        may exist! The full error is shown below.
        
        Workspace: 8978ed66-2f5f-e28c-b304-5ca6ef12ef7d
        Error: internal server error

Note that I switched back to the master branch and saw the exact same errors. So, I know that my code changes did not introduce these sporadic workspace deletion errors.

[rberlind]

I want to note that the Policy Set Versions API endpoint does not have LIST and DELETE operations. That is why they are not covered in policy_set_version.go or policy_set_version_test.go.

[rberlind]

I also want to make one more note about the only other response structure that uses any json structure fields (instead of only using jsonapi structure fields), TestAccountDetails in testing.go. It has the following structure that mixes json and jsonapi structure tags

type TestAccountDetails struct {
	ID       string `json:"id" jsonapi:"primary,users"`
	Username string `jsonapi:"attr,username"`
	Email    string `jsonapi:"attr,email"`
}

I wonder if we should be using the following in that structure for the ID?

ID       string `jsonapi:"primary,users"`

I have tested with that and all tests passed except some Run tests. But those only give errors about workspaces not being deleted and sometimes fail with the original code.

The only other reference I could find to mixing json and jsonapi tags is google/jsonapi#45 in which one of the main contributors suggests that mixing tags is probably not desirable.

Note that in my modified version of the do() method in tfe.go, I do the following check:

if jsonApiFields == 0 {
	return json.NewDecoder(resp.Body).Decode(v)
} else {
	return jsonapi.UnmarshalPayload(resp.Body, v)
}

Originally, I had used if jsonFields > 0, matching the check done in the serializeRequestBody() method of tfe.go. But that gave errors for tests that used TestAccountDetails since those tests then used the first option above instead of the jsonapi.UnmarshalPayload() function.

Changing TestAccountDetails to use

ID       string `jsonapi:"primary,users"`

would allow me to use if jsonFields > 0.

I originally also included this check to prevent simultaneous use of json and jsonapi structure fields:

if jsonApiFields > 0 && jsonFields > 0 {
    return nil, errors.New("go-tfe bug: response struct can't use both json and jsonapi attributes")
}

That mirrored what the serializeRequestBody() function of tfe.go does. But I removed this check since I then got that error in connection with TestAccountDetails.

So, if we did change TestAccountDetails as I suggest above, I could then add that check back and also use if jsonFields > 0 instead of if jsonApiFields == 0.

[chrisarcand]

Hi Roger!

First off, I just wanted to clarify: while acknowledging your intent to add support to the TFE provider is great, please note that this client is separate from the provider and not all APIs that are included in one make sense in the other. Happy to continue the conversation over in the provider, but all feedback here is with the client in mind unless stated otherwise.

Overall, I think adding an API for this client for PSVs sounds great! Before doing a detailed review line-by-line, there are some high-level considerations to address first, as they may significantly change your implementation:

• The separate Upload() API provides no value. If I must explicitly create the PSV, and then explicitly access and pass the blob storage URL to then feed to Upload(), I'm basically doing nothing more than the usual manual steps one would do with a simple PUT call - which coincidentally is all Upload() does. A far better API for this client would be to do the heavy lifting one would do manually in fully creating a useful PSV: Create() should accept a path to upload the artifact you wish to use with your created PSV in one action.
• The custom decoder logic - while you've proven its necessity - is a big change and making single PSV values a sole exception and forgoing the jsonapi decoder seems unwise. In addition, the logic here seems to be incomplete; a links member may be present in several areas beyond a single resource document: an index of documents, in included, etc) - but only single resource documents are handled in the decoding logic here. In other words:
  • The necessary scope of this is larger than you think and merits a separate prerequisite PR altogether, sadly.
  • If possible - and I believe it should be - we should strive to unmarshal PSVs with the standard jsonapi decoder as is done with every other resource, and do some additional custom unmarshalling for links values if necessary as a secondary step.

[rberlind]

Thanks for your feedback, @chrisarcand .

Regarding the separate Upload() function in addition to the Create() function, I was emulating what the Policy Set Versions API endpoint of the TFE API has in https://www.terraform.io/docs/cloud/api/policy-sets.html#create-a-policy-set-version and https://www.terraform.io/docs/cloud/api/policy-sets.html#upload-policy-set-versions. I agree with you that having a single Create() function that would do the upload makes sense. In fact, that is what I did for the resourceTFEPolicySetVersionCreate() function in my PR for the TFE provider itself. In any case, I would be happy to move the uploading done by my Upload() function into the Create() function. I could do that and then modify my PR for the TFE Client to only call that function instead of calling both Create() and Upload().

Regarding my use of json instead of jsonapi in the PSV structure and my customization of the do() function in tfe.go, I would also rather thave used the jsonapi structure tags. But I could not figure out how to do it. It is true that my specific issue related to the links member of the PSV structure, but my modifications of the do() function are not specific to the presence of a links member. Those modifications just support use of structures that only use json structure tags instead of jsonapi structure tags. I agree it is a big change as far as decoding responses is concerned, but it seemed to have precedent to me in so far as the serializeRequestBody() function also has logic to handle json structure tags instead of jsonapi structure tags. But I do appreciate the point I think you're making which is that changes to the do() method to support json instead of jsonapi (if really needed) should be done in a way that would work for future objects that might also use json tags in various ways.

You suggested it should be possible to unmarshal PSVs with the standard jsonapi decoder and do some additional custom unmarshalling for links values if necessary as a secondary step. If you can show me how to do that, I would happily use that approach.

Thanks again,
Roger

[chrisarcand]

Something I just stumbled upon that made me think of this is the synonymous API for registry modules, and how it creates versions - which is not what I suggested above:

go-tfe/registry_module.go

Lines 21 to 22 in 0598e3a

	// Create a registry module version
	CreateVersion(ctx context.Context, organization string, name string, provider string, options RegistryModuleCreateVersionOptions) (*RegistryModuleVersion, error)

This [version] API could be emulated for consistency. It also simplifies and provides flexibility for users by not taking on the responsibility of uploading a separate file - if the source is indeed from a file, the caller can do that - but if it's not and being pulled programmatically somewhere else, that's of no concern to the client, who just wants the source representation. Coincidentally, this is exactly what's being asked for with ConfigurationVersions in #131 , which I note is also what you probably emulated this existing implementation on.

[rberlind]

I'm a bit confused, @chrisarcand . I looked at https://github.com/hashicorp/go-tfe/blob/master/registry_module.go and see that it includes the CreateVersion() function, but I cannot find any indication that the code is able to return the upload link to the user. I certainly do not see an upload link within the RegistryModuleVersion structure. Perhaps you're suggesting that I should not be creating a separate policy_set_version.go and should instead be adding my logic to policy_set.go? I could do that, But I would still need help figuring out how to use the jsonapi structure tags for links instead of json structure tags.

You might also be saying that the go-tfe library should not even be handling uploading policy set versions, configuration versions, etc. I imagine that would mean that go-tfe would only provide a user who had created a policy set version, a module version, or a configuration version with the upload URL they need to use to upload the relevant files themselves. However, if this is what you're suggesting, we still need to figure out how to provide the user the actual upload URLs in all cases. That being said, for my use case of adding tfe_policy_set_version to the TFE provider, if we do not provide a way of uploading the policy set version files in go-tfe, I would then have to code the uploading into that resource itself and I would not necessarily want to support multiple upload options in the initial version.

I believe I did copy some code from configuration_version.go including the creation of separate Create() and Upload() methods. However, while the Configuration Version API returns upload-url as an attribute, the PolicySet Version API only returns it as a link. One way around the poor support for deserializing links in the jsonapi package would be for us to add an additional upload-url attribute to the data returned by the Policy Set Versions API (while keeping the upload link for backwards compatibility).

[rberlind]

Closing since #230 implemented what I had done.