Repository: opentofu/opentofu
File: internal/encryption/method/external/compliance_test.go (Go)

Code before:
// SPDX-License-Identifier: MPL-2.0

package external

import (
	"fmt"
	"slices"
	"strings"
	"testing"

	"github.com/opentofu/opentofu/internal/encryption/keyprovider"
	"github.com/opentofu/opentofu/internal/encryption/method/compliancetest"
	"github.com/opentofu/opentofu/internal/encryption/method/external/testmethod"
)

func TestComplianceBinary(t *testing.T) {
	runTest(t, testmethod.Go(t))
}

func TestCompliancePython(t *testing.T) {
	runTest(t, testmethod.Python(t))
}

func runTest(t *testing.T, cmd []string) {
	encryptCommand := append(cmd, "--encrypt")
	decryptCommand := append(cmd, "--decrypt")

	compliancetest.ComplianceTest(t, compliancetest.TestConfiguration[*descriptor, *Config, *command]{
		Descriptor: New().(*descriptor), //nolint:errcheck //This is safe.
		HCLParseTestCases: map[string]compliancetest.HCLParseTestCase[*descriptor, *Config, *command]{
			"empty": {
				HCL:        `method "external" "foo" {}`,
				ValidHCL:   false,
				ValidBuild: false,
				Validate:   nil,
			},
			"empty-command": {
				HCL: `method "external" "foo" {
	encrypt_command = []
	decrypt_command = []
}`,
				ValidHCL: true,
			},
			"command": {
				HCL: fmt.Sprintf(`method "external" "foo" {
	encrypt_command = ["%s"]
	decrypt_command = ["%s"]
}`, strings.Join(encryptCommand, `","`), strings.Join(decryptCommand, `","`)),
				ValidHCL:   true,
				ValidBuild: true,
				Validate: func(config *Config, method *command) error {

Review comment (bug):
While I agree with removing this `nolint` comment, I do find these two statements a little suspicious:

If `len(cmd) == cap(cmd)`  here then this would cause `encryptCommand` and `decryptCommand` to both have newly-allocated backing arrays that both share a prefix with `cmd`.

But if `len(cmd) < cap(cmd)` then I think these would both end up with the same backing array and the last item set to `--decrypt`, which is presumably wrong.

Both callers of this function seem to populate `cmd` using a composite literal:

https://github.com/opentofu/opentofu/blob/ec4e0cf0e2cefcca1d2ace792416c4e584c440a6/internal/encryption/method/external/testmethod/testmethod.go#L58

https://github.com/opentofu/opentofu/blob/ec4e0cf0e2cefcca1d2ace792416c4e584c440a6/internal/encryption/method/external/testmethod/testmethod.go#L76

...and the Go spec doesn't seem to specify what the capacity is of a slice created with a slice literal, but some experimentation on the Go playground suggests that, at least for small slices, the composite literal syntax currently allocates a backing array exactly the right size for the length of the slice, which I think is saving us from the bug that the linter was trying to warn about here. Therefore this currently works but is at risk of becoming accidentally broken in future if the callers start building these slices in a different way.

One way to resolve this while still retaining a similar code style would be to explicitly remove any excess capacity from the source slice before appending to it, and now we've upgraded to Go 1.24 there's [a helper function that's (subjectively) more readable than the three-clause slice syntax](https://pkg.go.dev/slices#Clip):

```go
cmd = slices.Clip(cmd)
encryptCommand := append(cmd, "--encrypt")
decryptCommand := append(cmd, "--decrypt")
```

Code after:
// SPDX-License-Identifier: MPL-2.0

package external

import (
	"fmt"
	"slices"
	"strings"
	"testing"

	"github.com/opentofu/opentofu/internal/encryption/keyprovider"
	"github.com/opentofu/opentofu/internal/encryption/method/compliancetest"
	"github.com/opentofu/opentofu/internal/encryption/method/external/testmethod"
)

func TestComplianceBinary(t *testing.T) {
	runTest(t, testmethod.Go(t))
}

func TestCompliancePython(t *testing.T) {
	runTest(t, testmethod.Python(t))
}

func runTest(t *testing.T, cmd []string) {
	cmd = slices.Clip(cmd) // Make sure that the following appends are forced to allocate capacity
	encryptCommand := append(cmd, "--encrypt")
	decryptCommand := append(cmd, "--decrypt")

	compliancetest.ComplianceTest(t, compliancetest.TestConfiguration[*descriptor, *Config, *command]{
		Descriptor: New().(*descriptor), //nolint:errcheck //This is safe.
		HCLParseTestCases: map[string]compliancetest.HCLParseTestCase[*descriptor, *Config, *command]{
			"empty": {
				HCL:        `method "external" "foo" {}`,
				ValidHCL:   false,
				ValidBuild: false,
				Validate:   nil,
			},
			"empty-command": {
				HCL: `method "external" "foo" {
	encrypt_command = []
	decrypt_command = []
}`,
				ValidHCL: true,
			},
			"command": {
				HCL: fmt.Sprintf(`method "external" "foo" {
	encrypt_command = ["%s"]
	decrypt_command = ["%s"]
}`, strings.Join(encryptCommand, `","`), strings.Join(decryptCommand, `","`)),
				ValidHCL:   true,
				ValidBuild: true,