ronantakizawa/codereview-bench huggingface dataset:
Repository: opentofu/opentofu
File: internal/backend/remote-state/oss/backend_test.go (Go)

Code before:
			expectedProxyURL: "",
		},
		{
			name:             "should not set https proxy if NO_PROXY contains the host",
			rawUrl:           "https://secure.internal",
			httpProxy:        "",
			httpsProxy:       "https://foo.baz:3128",
			noProxy:          "secure.internal",
			expectedProxyURL: "",
		},
		{
			name:             "should not set https proxy if NO_PROXY matches the domain with suffix",
			rawUrl:           "https://ss.qcsc.secure.internal",
			httpProxy:        "",
			httpsProxy:       "https://foo.baz:3128",
			noProxy:          ".secure.internal",
			expectedProxyURL: "",
		},
	}

	for _, tt := range tests {
		t.Run(tt.name, func(t *testing.T) {
			// Set environment variables
			os.Setenv("HTTP_PROXY", tt.httpProxy)
			os.Setenv("HTTPS_PROXY", tt.httpsProxy)
			os.Setenv("NO_PROXY", tt.noProxy)

			proxyUrl, err := getHttpProxyUrl(tt.rawUrl)
			if err != nil {
				t.Fatalf("unexpected error: %s", err)
			}

			if tt.expectedProxyURL == "" {
				if proxyUrl != nil {
					t.Fatalf("unexpected proxy  URL, want nil, got: %s", proxyUrl)
				}
			} else {
				if tt.expectedProxyURL != proxyUrl.String() {
					t.Fatalf("unexpected proxy URL, want: %s, got: %s", tt.expectedProxyURL, proxyUrl.String())
				}
			}
		})
	}
}


Review comment (suggestion):
I think we can use the testing-specific [`t.SetEnv`](https://pkg.go.dev/testing#T.Setenv) here to make sure that these environment variable changes will be undone once the test is complete.


Code after:
			expectedProxyURL: "",
		},
		{
			name:             "should not set https proxy if NO_PROXY contains the host",
			rawUrl:           "https://secure.internal",
			httpProxy:        "",
			httpsProxy:       "https://foo.baz:3128",
			noProxy:          "secure.internal",
			expectedProxyURL: "",
		},
		{
			name:             "should not set https proxy if NO_PROXY matches the domain with suffix",
			rawUrl:           "https://ss.qcsc.secure.internal",
			httpProxy:        "",
			httpsProxy:       "https://foo.baz:3128",
			noProxy:          ".secure.internal",
			expectedProxyURL: "",
		},
	}

	for _, tt := range tests {
		t.Run(tt.name, func(t *testing.T) {
			// Set environment variables
			t.Setenv("HTTP_PROXY", tt.httpProxy)
			t.Setenv("HTTPS_PROXY", tt.httpsProxy)
			t.Setenv("NO_PROXY", tt.noProxy)

			proxyUrl, err := getHttpProxyUrl(tt.rawUrl)
			if err != nil {
				t.Fatalf("unexpected error: %s", err)
			}

			if tt.expectedProxyURL == "" {
				if proxyUrl != nil {
					t.Fatalf("unexpected proxy  URL, want nil, got: %s", proxyUrl)
				}
			} else {
				if tt.expectedProxyURL != proxyUrl.String() {
					t.Fatalf("unexpected proxy URL, want: %s, got: %s", tt.expectedProxyURL, proxyUrl.String())
				}
			}
		})
	}
}
Repository: opentofu/opentofu
File: internal/tracing/traceattrs/traceattrs.go (Go)

Code before:
// Copyright (c) The OpenTofu Authors
// SPDX-License-Identifier: MPL-2.0
// Copyright (c) 2023 HashiCorp, Inc.
// SPDX-License-Identifier: MPL-2.0

package traceattrs

const (
	// Common attributes names used across the codebase

	ProviderAddress = "opentofu.provider.address"
	ProviderVersion = "opentofu.provider.version"

	TargetPlatform = "opentofu.target_platform"

	ModuleName    = "opentofu.module.name"
	ModuleAddress = "opentofu.module.address"
	ModuleVersion = "opentofu.module.version"
)


Review comment (suggestion):
From observing how these are being used in the other code, perhaps these slightly different names will correlate better with terminology we use elsewhere in the UI/docs:

```suggestion
	ModuleCallName = "opentofu.module.call_name"
	ModuleSource   = "opentofu.module.source"
	ModuleVersion  = "opentofu.module.version"
```

(Of these "module call" is the one we're currently least consistent about -- the docs mostly use it as a verb "call a module" rather than a noun "the module call" (referring to a `module` block) -- but "module call" is how a bunch of our internal code already refers to this, and I don't have any better way to distinguish between the block itself and the module that the block instantiates.) :thinking: 

I notice that you already used "module source" to refer to what I might instead call a source _type_ -- local, registry, or remote. This is an interesting situation where the internal architecture and the user experience are inconsistent with one another, since "remote" is mostly an implementation detail that means "something we install using go-getter". If we want to keep to our principle of preferring user-facing concepts over implementation concepts then I might suggest to set `opentofu.module.source_type` to be either "registry", "local", or the chosen getter's name, such as "git", "http", "oci", etc, instead of exposing the generic "remote"... but I don't know if we have that information in all of the places that we'd actually want to record it.



Code after:
// Copyright (c) The OpenTofu Authors
// SPDX-License-Identifier: MPL-2.0
// Copyright (c) 2023 HashiCorp, Inc.
// SPDX-License-Identifier: MPL-2.0

package traceattrs

const (
	// Common attributes names used across the codebase

	ProviderAddress = "opentofu.provider.address"
	ProviderVersion = "opentofu.provider.version"

	TargetPlatform = "opentofu.target_platform"

	ModuleCallName = "opentofu.module.name"
	ModuleSource   = "opentofu.module.source"
	ModuleVersion  = "opentofu.module.version"
)
Repository: opentofu/opentofu
File: internal/backend/remote-state/azure/backend_state.go (Go)

Code before:
	count := 1
	envs := map[string]struct{}{}
	prefix := b.keyName + keyEnvPrefix
	params := containers.ListBlobsInput{
		Prefix: &prefix,
	}

	for {

		log.Printf("[TRACE] Getting page %d of blob results", count)
		resp, err := client.ListBlobs(ctx, b.armClient.storageAccountName, b.containerName, params)
		if err != nil {
			return nil, err
		}

		params.Marker = resp.NextMarker
		for _, obj := range resp.Blobs.Blobs {
			key := obj.Name
			if strings.HasPrefix(key, prefix) {
				name := strings.TrimPrefix(key, prefix)
				// we store the state in a key, not a directory
				if strings.Contains(name, "/") {
					continue
				}
				envs[name] = struct{}{}
			}
		}

		count++
		if *resp.NextMarker == "" {
			break;
		}
	}

	result := []string{backend.DefaultStateName}
	for name := range envs {
		result = append(result, name)
	}
	sort.Strings(result[1:])
	return result, nil
}

func (b *Backend) DeleteWorkspace(name string, _ bool) error {
	if name == backend.DefaultStateName || name == "" {
		return fmt.Errorf("can't delete default state")
	}

	ctx := context.TODO()
	client, err := b.armClient.getBlobClient(ctx)
	if err != nil {
		return err

Review comment (suggestion):
Easier to read.
```suggestion
			if !strings.HasPrefix(key, prefix) {
			  continue
			}
			name := strings.TrimPrefix(key, prefix)
			// we store the state in a key, not a directory
			if strings.Contains(name, "/") {
				continue
			}
			envs[name] = struct{}{}
```

Code after:
	defer cancel()

	client, err := b.armClient.getContainersClient(ctx)
	if err != nil {
		return nil, err
	}

	prefix := b.keyName + keyEnvPrefix
	result, err := getPaginatedResults(ctx, client, prefix, b.armClient.storageAccountName, b.containerName)
	if err != nil {
		return nil, err
	}

	return result, nil
}

func (b *Backend) DeleteWorkspace(name string, _ bool) error {
	if name == backend.DefaultStateName || name == "" {
		return fmt.Errorf("can't delete default state")
	}

	ctx, cancel := b.getContextWithTimeout()
	defer cancel()
	client, err := b.armClient.getBlobClient(ctx)
	if err != nil {
		return err
	}

	if resp, err := client.Delete(ctx, b.armClient.storageAccountName, b.containerName, b.path(name), blobs.DeleteInput{}); err != nil {
		if resp.Response.StatusCode != 404 {
			return err
		}
	}

	return nil
}

func (b *Backend) StateMgr(name string) (statemgr.Full, error) {
	ctx, cancel := b.getContextWithTimeout()
	defer cancel()

	blobClient, err := b.armClient.getBlobClient(ctx)
	if err != nil {
		return nil, err
	}

	client := &RemoteClient{
		giovanniBlobClient: *blobClient,
		containerName:      b.containerName,
		keyName:            b.path(name),
		accountName:        b.accountName,
Repository: opentofu/opentofu
File: internal/tofu/schemas.go (Go)

Code before:
		}

		schemas[fqn] = schema
	}

	if config != nil {
		for _, fqn := range config.ProviderTypes() {
			ensure(fqn)
		}
	}

	if state != nil {
		needed := providers.AddressedTypesAbs(state.ProviderAddrs())
		for _, typeAddr := range needed {
			ensure(typeAddr)
		}
	}

	return diags
}

func loadProvisionerSchemas(ctx context.Context, schemas map[string]*configschema.Block, config *configs.Config, plugins *contextPlugins) tfdiags.Diagnostics {
	var diags tfdiags.Diagnostics

	ensure := func(name string) {
		_, span := tracing.Tracer().Start(ctx, "Get Provisioner Schema")
		defer span.End()
		if _, exists := schemas[name]; exists {
			return
		}

		log.Printf("[TRACE] LoadSchemas: retrieving schema for provisioner %q", name)
		schema, err := plugins.ProvisionerSchema(name)
		if err != nil {
			// We'll put a stub in the map so we won't re-attempt this on
			// future calls, which would then repeat the same error message
			// multiple times.
			schemas[name] = &configschema.Block{}
			diags = diags.Append(
				tfdiags.Sourceless(
					tfdiags.Error,
					"Failed to obtain provisioner schema",
					fmt.Sprintf("Could not load the schema for provisioner %q: %s.", name, err),
				),
			)
			tracing.SetSpanError(span, diags)
			return
		}

		schemas[name] = schema
	}

Review comment (suggestion):
Will `ProvisionerSchema` receive `context.Context` too? So we should expect the ctx from this `Start()` call to be added back when `ProvisionerSchema` will accept it?
Asking because in the PRs linked in the description, this is not yet included and wondering if it's planned to.

Code after:
		needed := providers.AddressedTypesAbs(state.ProviderAddrs())
		for _, typeAddr := range needed {
			ensure(typeAddr)
		}
	}

	return diags
}

func loadProvisionerSchemas(ctx context.Context, schemas map[string]*configschema.Block, config *configs.Config, plugins *contextPlugins) tfdiags.Diagnostics {
	var diags tfdiags.Diagnostics

	ensure := func(name string) {
		log.Printf("[TRACE] LoadSchemas: retrieving schema for provisioner %q", name)
		schema, err := plugins.ProvisionerSchema(name)
		if err != nil {
			// We'll put a stub in the map so we won't re-attempt this on
			// future calls, which would then repeat the same error message
			// multiple times.
			schemas[name] = &configschema.Block{}
			diags = diags.Append(
				tfdiags.Sourceless(
					tfdiags.Error,
					"Failed to obtain provisioner schema",
					fmt.Sprintf("Could not load the schema for provisioner %q: %s.", name, err),
				),
			)

			return
		}

		schemas[name] = schema
	}

	if config != nil {
		for _, rc := range config.Module.ManagedResources {
			for _, pc := range rc.Managed.Provisioners {
				ensure(pc.Type)
			}
		}

		// Must also visit our child modules, recursively.
		for _, cc := range config.Children {
			childDiags := loadProvisionerSchemas(ctx, schemas, cc, plugins)
			diags = diags.Append(childDiags)
		}
	}

	return diags
}
Repository: opentofu/opentofu
File: internal/command/fmt.go (Go)

Code before:

	var output io.Writer
	list := c.list // preserve the original value of -list
	if c.check {
		// set to true so we can use the list output to check
		// if the input needs formatting
		c.list = true
		c.write = false
		output = &bytes.Buffer{}
	} else {
		output = &cli.UiWriter{Ui: c.Ui}
	}

	diags := c.fmt(paths, c.input, output)
	c.showDiagnostics(diags)
	if diags.HasErrors() {
		return 2
	}

	if c.check {
		buf := output.(*bytes.Buffer)
		ok := buf.Len() == 0
		if list {
			if _, err := io.Copy(&cli.UiWriter{Ui: c.Ui}, buf); err != nil {
				// Should never happen
				panic(err)
			}
		}
		if ok {
			return 0
		} else {
			return 3
		}
	}

	return 0
}

func (c *FmtCommand) fmt(paths []string, stdin io.Reader, stdout io.Writer) tfdiags.Diagnostics {
	var diags tfdiags.Diagnostics

	if len(paths) == 0 { // Assuming stdin, then.
		if c.write {
			diags = diags.Append(fmt.Errorf("Option -write cannot be used when reading from stdin"))
			return diags
		}
		fileDiags := c.processFile("<stdin>", stdin, stdout, true)
		diags = diags.Append(fileDiags)
		return diags
	}


Review comment (suggestion):
I understand that this is not meant to happen, but if ever does, this panic is changing the currently existing behaviour.
Wouldn't be better to just log an error instead?

Or even better, since this is already in the part of the method that returns an exit code, we could return a different exit code that we could document.

Code after:

	var output io.Writer
	list := c.list // preserve the original value of -list
	if c.check {
		// set to true so we can use the list output to check
		// if the input needs formatting
		c.list = true
		c.write = false
		output = &bytes.Buffer{}
	} else {
		output = &cli.UiWriter{Ui: c.Ui}
	}

	diags := c.fmt(paths, c.input, output)
	c.showDiagnostics(diags)
	if diags.HasErrors() {
		return 2
	}

	if c.check {
		buf := output.(*bytes.Buffer)
		ok := buf.Len() == 0
		if list {
			if _, err := io.Copy(&cli.UiWriter{Ui: c.Ui}, buf); err != nil {
				log.Printf("[ERROR] Unable to write UI output: %s", err)
			}
		}
		if ok {
			return 0
		} else {
			return 3
		}
	}

	return 0
}

func (c *FmtCommand) fmt(paths []string, stdin io.Reader, stdout io.Writer) tfdiags.Diagnostics {
	var diags tfdiags.Diagnostics

	if len(paths) == 0 { // Assuming stdin, then.
		if c.write {
			diags = diags.Append(fmt.Errorf("Option -write cannot be used when reading from stdin"))
			return diags
		}
		fileDiags := c.processFile("<stdin>", stdin, stdout, true)
		diags = diags.Append(fileDiags)
		return diags
	}

	for _, path := range paths {
Repository: opentofu/opentofu
File: internal/command/show.go (Go)

Code before:
	// Load the configuration
	config, configDiags := c.loadConfig(ctx, ".")
	diags = diags.Append(configDiags)
	if configDiags.HasErrors() {
		return nil, diags
	}

	// Load provider schemas (without state)
	schemas, schemaDiags := c.MaybeGetSchemas(ctx, nil, config)
	diags = diags.Append(schemaDiags)
	if schemaDiags.HasErrors() {
		return nil, diags
	}
	if schemas == nil {
		diags = diags.Append(tfdiags.Sourceless(
			tfdiags.Error,
			"Failed to load provider schemas",
			"The configuration cannot be shown without provider schema information.",
		))
		return nil, diags
	}

	// Return a function that will render the configuration as JSON
	return func(view views.Show) int {
		// Display the configuration using the view
		return view.(*views.ShowJSON).DisplayConfig(config, schemas)
	}, diags
}


Review comment (bug):
Could this just be `views.DisplayConfig`, without the type assertion, and let it fail with the error you added for the `ShowHuman` implementation of this method?

I expect that in practice this isn't reachable with anything other than the JSON view due to how the arguments parsing is implemented, but if possible I'd prefer for this part to be written similarly to the other two modes to minimize surprise for future maintainers.


Code after:
	if err != nil {
		return nil, fmt.Errorf("Error loading statefile: %w", err)
	}
	defer file.Close()

	var stateFile *statefile.File
	stateFile, err = statefile.Read(file, enc.State())
	if err != nil {
		return nil, fmt.Errorf("Error reading %s as a statefile: %w", path, err)
	}
	return stateFile, nil
}

// getStateFromBackend returns the State for the current workspace, if available.
func getStateFromBackend(ctx context.Context, b backend.Backend, workspace string) (*statefile.File, error) {
	ctx, span := tracing.Tracer().Start(ctx, "Get State from Backend")
	defer span.End()
	// Get the state store for the given workspace
	stateStore, err := b.StateMgr(ctx, workspace)
	if err != nil {
		tracing.SetSpanError(span, err)
		return nil, fmt.Errorf("failed to load state manager: %w", err)
	}

	// Refresh the state store with the latest state snapshot from persistent storage
	if err := stateStore.RefreshState(); err != nil {
		tracing.SetSpanError(span, err)
		return nil, fmt.Errorf("failed to load state: %w", err)
	}

	// Get the latest state snapshot and return it
	stateFile := statemgr.Export(stateStore)
	return stateFile, nil
}

// showConfiguration returns a function that will display the current configuration
// in JSON format. This is a new feature that requires -json to be specified.
func (c *ShowCommand) showConfiguration(ctx context.Context) (showRenderFunc, tfdiags.Diagnostics) {
	var diags tfdiags.Diagnostics

	// Check if the directory is empty
	empty, err := configs.IsEmptyDir(".")
	if err != nil {
		diags = diags.Append(tfdiags.Sourceless(
			tfdiags.Error,
			"Error validating configuration directory",
			fmt.Sprintf("OpenTofu encountered an unexpected error while verifying that the given configuration directory is valid: %s.", err),
		))
		return nil, diags
	}
	if empty {
Repository: opentofu/opentofu
File: internal/backend/remote-state/consul/client.go (Go)

Code before:
		return nil
	}

	return c.unlock(id)
}

// the unlock implementation.
// Only to be called while holding Client.mu
func (c *RemoteClient) unlock(id string) error {
	// This method can be called in two circumstances:
	// - when the plan apply or destroy operation finishes and the lock needs to be released,
	// the watchdog stopped and the session closed
	// - when the user calls `tofu force-unlock <lock_id>` in which case
	// we only need to release the lock.

	if c.consulLock == nil || c.lockCh == nil {
		// The user called `tofu force-unlock <lock_id>`, we just destroy
		// the session which will release the lock, clean the KV store and quit.

		_, err := c.Client.Session().Destroy(id, nil)
		if err != nil {
			return err
		}
		// We ignore the errors that may happen during cleanup
		kv := c.Client.KV()
		kv.Delete(c.lockPath()+lockSuffix, nil)     //nolint:errcheck
		kv.Delete(c.lockPath()+lockInfoSuffix, nil) //nolint:errcheck

		return nil
	}

	// cancel our monitoring goroutine
	c.monitorCancel()

	defer func() {
		c.consulLock = nil

		// The consul session is only used for this single lock, so cancel it
		// after we unlock.
		// The session is only created and replaced holding Client.mu, so the
		// CancelFunc must be non-nil.
		c.sessionCancel()
	}()

	select {
	case <-c.lockCh:
		return lostLockErr
	default:
	}

	kv := c.Client.KV()

Review comment (suggestion):
`suggestion`
We could at least log the errors if occur.

Code after:
		return nil
	}

	return c.unlock(id)
}

// the unlock implementation.
// Only to be called while holding Client.mu
func (c *RemoteClient) unlock(id string) error {
	// This method can be called in two circumstances:
	// - when the plan apply or destroy operation finishes and the lock needs to be released,
	// the watchdog stopped and the session closed
	// - when the user calls `tofu force-unlock <lock_id>` in which case
	// we only need to release the lock.

	if c.consulLock == nil || c.lockCh == nil {
		// The user called `tofu force-unlock <lock_id>`, we just destroy
		// the session which will release the lock, clean the KV store and quit.

		_, err := c.Client.Session().Destroy(id, nil)
		if err != nil {
			return err
		}
		// We ignore the errors that may happen during cleanup
		kv := c.Client.KV()
		_, err = kv.Delete(c.lockPath()+lockSuffix, nil)
		if err != nil {
			log.Printf("[ERROR] could not delete lock @ %s: %s\n", c.lockPath()+lockSuffix, err)
		}
		_, err = kv.Delete(c.lockPath()+lockInfoSuffix, nil)
		if err != nil {
			log.Printf("[ERROR] could not delete lock info @ %s: %s\n", c.lockPath()+lockInfoSuffix, err)
		}

		return nil
	}

	// cancel our monitoring goroutine
	c.monitorCancel()

	defer func() {
		c.consulLock = nil

		// The consul session is only used for this single lock, so cancel it
		// after we unlock.
		// The session is only created and replaced holding Client.mu, so the
		// CancelFunc must be non-nil.
		c.sessionCancel()
	}()

	select {
Repository: opentofu/opentofu
File: internal/backend/remote-state/consul/client.go (Go)

Code before:
	select {
	case <-c.lockCh:
		return lostLockErr
	default:
	}

	kv := c.Client.KV()

	var errs error

	if _, err := kv.Delete(c.lockPath()+lockInfoSuffix, nil); err != nil {
		errs = multierror.Append(errs, err)
	}

	if err := c.consulLock.Unlock(); err != nil {
		errs = multierror.Append(errs, err)
	}

	// the monitoring goroutine may be in a select on the lockCh, so we need to
	// wait for it to return before changing the value.
	c.monitorWG.Wait()
	c.lockCh = nil

	// This is only cleanup, and will fail if the lock was immediately taken by
	// another client, so we don't report an error to the user here.
	c.consulLock.Destroy() //nolint:errcheck

	return errs
}

func compressState(data []byte) ([]byte, error) {
	b := new(bytes.Buffer)
	gz := gzip.NewWriter(b)
	if _, err := gz.Write(data); err != nil {
		return nil, err
	}
	if err := gz.Flush(); err != nil {
		return nil, err
	}
	if err := gz.Close(); err != nil {
		return nil, err
	}
	return b.Bytes(), nil
}

func uncompressState(data []byte) ([]byte, error) {
	b := new(bytes.Buffer)
	gz, err := gzip.NewReader(bytes.NewReader(data))
	if err != nil {
		return nil, err
	}

Review comment (suggestion):
`suggestion`
We could at least log the error if occurs.

Code after:
		// after we unlock.
		// The session is only created and replaced holding Client.mu, so the
		// CancelFunc must be non-nil.
		c.sessionCancel()
	}()

	select {
	case <-c.lockCh:
		return lostLockErr
	default:
	}

	kv := c.Client.KV()

	var errs error

	if _, err := kv.Delete(c.lockPath()+lockInfoSuffix, nil); err != nil {
		errs = multierror.Append(errs, err)
	}

	if err := c.consulLock.Unlock(); err != nil {
		errs = multierror.Append(errs, err)
	}

	// the monitoring goroutine may be in a select on the lockCh, so we need to
	// wait for it to return before changing the value.
	c.monitorWG.Wait()
	c.lockCh = nil

	// This is only cleanup, and will fail if the lock was immediately taken by
	// another client, so we don't report an error to the user here.
	err := c.consulLock.Destroy()
	if err != nil {
		log.Printf("[ERROR] could not destroy consul lock: %s\n", err)
	}

	return errs
}

func compressState(data []byte) ([]byte, error) {
	b := new(bytes.Buffer)
	gz := gzip.NewWriter(b)
	if _, err := gz.Write(data); err != nil {
		return nil, err
	}
	if err := gz.Flush(); err != nil {
		return nil, err
	}
	if err := gz.Close(); err != nil {
		return nil, err
	}