fix(file): properly handle overwrite option behavior in proxmox_virtual_environment_download_file (#1989)

Signed-off-by: rafsaf <rafal.safin@rafsaf.pl>

docs/resources/virtual_environment_download_file.md

@@ -84,7 +84,7 @@ resource "proxmox_virtual_environment_download_file" "latest_ubuntu_22_jammy_lxc
 - `checksum_algorithm` (String) The algorithm to calculate the checksum of the file. Must be `md5` | `sha1` | `sha224` | `sha256` | `sha384` | `sha512`.
 - `decompression_algorithm` (String) Decompress the downloaded file using the specified compression algorithm. Must be one of `gz` | `lzo` | `zst` | `bz2`.
 - `file_name` (String) The file name. If not provided, it is calculated using `url`. PVE will raise 'wrong file extension' error for some popular extensions file `.raw` or `.qcow2`. Workaround is to use e.g. `.img` instead.
-- `overwrite` (Boolean) If `true` and size of uploaded file is different, than size from `url` Content-Length header, file will be downloaded again. If `false`, there will be no checks.
+- `overwrite` (Boolean) By default `true`. If `true` and file size has changed in the datastore, it will be replaced. If `false`, there will be no check.
 - `overwrite_unmanaged` (Boolean) If `true` and a file with the same name already exists in the datastore, it will be deleted and the new file will be downloaded. If `false` and the file already exists, an error will be returned.
 - `upload_timeout` (Number) The file download timeout seconds. Default is 600 (10min).
 - `verify` (Boolean) By default `true`. If `false`, no SSL/TLS certificates will be verified.
@@ -92,4 +92,4 @@ resource "proxmox_virtual_environment_download_file" "latest_ubuntu_22_jammy_lxc
 ### Read-Only
 
 - `id` (String) The unique identifier of this resource.
-- `size` (Number) The file size.
+- `size` (Number) The file size in PVE.

fwprovider/nodes/resource_download_file.go

@@ -26,7 +26,6 @@ import (
 	"github.com/hashicorp/terraform-plugin-framework/resource/schema/stringplanmodifier"
 	"github.com/hashicorp/terraform-plugin-framework/schema/validator"
 	"github.com/hashicorp/terraform-plugin-framework/types"
-	"github.com/hashicorp/terraform-plugin-log/tflog"
 
 	"github.com/bpg/terraform-provider-proxmox/fwprovider/attribute"
 	"github.com/bpg/terraform-provider-proxmox/fwprovider/config"
@@ -74,11 +73,10 @@ func (r sizeRequiresReplaceModifier) PlanModifyInt64(
 		originalStateSize, err := strconv.ParseInt(string(originalStateSizeBytes), 10, 64)
 		if err != nil {
 			resp.Diagnostics.AddError(
-				"Unexpected error when reading originalStateSize from Private",
+				"Unable to convert original state file size to int64",
-				fmt.Sprintf(
+				"Unexpected error in parsing string to int64, key original_state_size. "+
-					"Unexpected error in ParseInt: %s",
+					"Please retry the operation or report this issue to the provider developers.\n\n"+
-					err.Error(),
+					"Error: "+err.Error(),
-				),
 			)
 
 			return
@@ -89,9 +87,10 @@ func (r sizeRequiresReplaceModifier) PlanModifyInt64(
 			resp.PlanValue = types.Int64Value(originalStateSize)
 
 			resp.Diagnostics.AddWarning(
-				"The file size in datastore has changed.",
+				"The file size in datastore has changed outside of terraform.",
 				fmt.Sprintf(
-					"Previous size %d does not match size from datastore: %d",
+					"Previous size: %d saved in state does not match current size from datastore: %d. "+
+						"You can disable this behaviour by using overwrite=false",
 					originalStateSize,
 					state.Size.ValueInt64(),
 				),
@@ -100,53 +99,6 @@ func (r sizeRequiresReplaceModifier) PlanModifyInt64(
 			return
 		}
 	}
-
-	urlSizeBytes, diags := req.Private.GetKey(ctx, "url_size")
-
-	resp.Diagnostics.Append(diags...)
-
-	if (urlSizeBytes != nil) && (plan.URL.ValueString() == state.URL.ValueString()) {
-		urlSize, err := strconv.ParseInt(string(urlSizeBytes), 10, 64)
-		if err != nil {
-			resp.Diagnostics.AddError(
-				"Unexpected error when reading urlSize from Private",
-				fmt.Sprintf(
-					"Unexpected error in ParseInt: %s",
-					err.Error(),
-				),
-			)
-
-			return
-		}
-
-		if state.Size.ValueInt64() != urlSize {
-			if urlSize < 0 {
-				resp.Diagnostics.AddWarning(
-					"Could not read the file metadata from URL.",
-					fmt.Sprintf(
-						"The remote file at URL %q most likely doesn’t exist or can’t be accessed.\n"+
-							"To skip the remote file check, set `overwrite` to `false`.",
-						plan.URL.ValueString(),
-					),
-				)
-			} else {
-				resp.RequiresReplace = true
-				resp.PlanValue = types.Int64Value(urlSize)
-
-				resp.Diagnostics.AddWarning(
-					"The file size from url has changed.",
-					fmt.Sprintf(
-						"Size %d from url %q does not match size from datastore: %d",
-						urlSize,
-						plan.URL.ValueString(),
-						state.Size.ValueInt64(),
-					),
-				)
-			}
-
-			return
-		}
-	}
 }
 
 func (r sizeRequiresReplaceModifier) Description(_ context.Context) string {
@@ -242,13 +194,12 @@ func (r *downloadFileResource) Schema(
 				},
 			},
 			"size": schema.Int64Attribute{
-				Description: "The file size.",
+				Description: "The file size in PVE.",
 				Optional:    false,
 				Required:    false,
 				Computed:    true,
 				PlanModifiers: []planmodifier.Int64{
 					int64planmodifier.UseStateForUnknown(),
-					int64planmodifier.RequiresReplace(),
 					sizeRequiresReplaceModifier{},
 				},
 			},
@@ -284,6 +235,9 @@ func (r *downloadFileResource) Schema(
 					"specified compression algorithm. Must be one of `gz` | `lzo` | `zst` | `bz2`.",
 				Optional: true,
 				Default:  nil,
+				PlanModifiers: []planmodifier.String{
+					stringplanmodifier.RequiresReplace(),
+				},
 				Validators: []validator.String{
 					stringvalidator.OneOf([]string{
 						"gz",
@@ -320,9 +274,8 @@ func (r *downloadFileResource) Schema(
 				Default:     booldefault.StaticBool(true),
 			},
 			"overwrite": schema.BoolAttribute{
-				Description: "If `true` and size of uploaded file is different, " +
+				Description: "By default `true`. If `true` and file size has changed in the datastore, " +
-					"than size from `url` Content-Length header, file will be downloaded again. " +
+					"it will be replaced. If `false`, there will be no check.",
-					"If `false`, there will be no checks.",
 				Optional: true,
 				Computed: true,
 				Default:  booldefault.StaticBool(true),
@@ -554,25 +507,6 @@ func (r *downloadFileResource) Read(
 		return
 	}
 
-	if state.Overwrite.ValueBool() {
-		// with overwrite, use url to get proper target size
-		urlMetadata, err := r.getURLMetadata(
-			ctx,
-			&state,
-		)
-		if err != nil {
-			tflog.Error(ctx, "Could not get file metadata from url", map[string]interface{}{
-				"error": err,
-				"url":   state.URL.ValueString(),
-			})
-			// force size to -1, which is a special value used in sizeRequiresReplaceModifier
-			resp.Private.SetKey(ctx, "url_size", []byte("-1"))
-		} else if urlMetadata.Size != nil {
-			setValue := []byte(strconv.FormatInt(*urlMetadata.Size, 10))
-			resp.Private.SetKey(ctx, "url_size", setValue)
-		}
-	}
-
 	resp.Diagnostics.Append(resp.State.Set(ctx, state)...)
 }
 

fwprovider/nodes/resource_download_file_test.go

@@ -263,11 +263,12 @@ func uploadIsoFile(t *testing.T, fileName string) {
 
 	sshAgent := utils.GetAnyBoolEnv("PROXMOX_VE_SSH_AGENT")
 	sshUsername := utils.GetAnyStringEnv("PROXMOX_VE_SSH_USERNAME")
+	sshPassword := utils.GetAnyStringEnv("PROXMOX_VE_SSH_PASSWORD")
 	sshAgentSocket := utils.GetAnyStringEnv("SSH_AUTH_SOCK", "PROXMOX_VE_SSH_AUTH_SOCK")
 	sshPrivateKey := utils.GetAnyStringEnv("PROXMOX_VE_SSH_PRIVATE_KEY")
 	sshPort := utils.GetAnyIntEnv("PROXMOX_VE_ACC_NODE_SSH_PORT")
 	sshClient, err := ssh.NewClient(
-		sshUsername, "", sshAgent, sshAgentSocket, sshPrivateKey,
+		sshUsername, sshPassword, sshAgent, sshAgentSocket, sshPrivateKey,
 		"", "", "",
 		&nodeResolver{
 			node: ssh.ProxmoxNode{

fwprovider/test/resource_file_test.go

@@ -252,11 +252,12 @@ func uploadSnippetFile(t *testing.T, fileName string) {
 
 	sshAgent := utils.GetAnyBoolEnv("PROXMOX_VE_SSH_AGENT")
 	sshUsername := utils.GetAnyStringEnv("PROXMOX_VE_SSH_USERNAME")
+	sshPassword := utils.GetAnyStringEnv("PROXMOX_VE_SSH_PASSWORD")
 	sshAgentSocket := utils.GetAnyStringEnv("SSH_AUTH_SOCK", "PROXMOX_VE_SSH_AUTH_SOCK")
 	sshPrivateKey := utils.GetAnyStringEnv("PROXMOX_VE_SSH_PRIVATE_KEY")
 	sshPort := utils.GetAnyIntEnv("PROXMOX_VE_ACC_NODE_SSH_PORT")
 	sshClient, err := ssh.NewClient(
-		sshUsername, "", sshAgent, sshAgentSocket, sshPrivateKey,
+		sshUsername, sshPassword, sshAgent, sshAgentSocket, sshPrivateKey,
 		"", "", "",
 		&nodeResolver{
 			node: ssh.ProxmoxNode{