Skip to content

Commit 1b86a41

Browse files
rafsafrafsaf
authored
fix(file): properly handle overwrite option behavior in proxmox_virtual_environment_download_file (#1989)
Signed-off-by: rafsaf <[email protected]>
1 parent 41f35e6 commit 1b86a41

File tree

4 files changed

+19
-83
lines changed

4 files changed

+19
-83
lines changed

‎docs/resources/virtual_environment_download_file.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -84,12 +84,12 @@ resource "proxmox_virtual_environment_download_file" "latest_ubuntu_22_jammy_lxc
8484
- `checksum_algorithm` (String) The algorithm to calculate the checksum of the file. Must be `md5` | `sha1` | `sha224` | `sha256` | `sha384` | `sha512`.
8585
- `decompression_algorithm` (String) Decompress the downloaded file using the specified compression algorithm. Must be one of `gz` | `lzo` | `zst` | `bz2`.
8686
- `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.
87-
- `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.
87+
- `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.
8888
- `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.
8989
- `upload_timeout` (Number) The file download timeout seconds. Default is 600 (10min).
9090
- `verify` (Boolean) By default `true`. If `false`, no SSL/TLS certificates will be verified.
9191

9292
### Read-Only
9393

9494
- `id` (String) The unique identifier of this resource.
95-
- `size` (Number) The file size.
95+
- `size` (Number) The file size in PVE.

‎fwprovider/nodes/resource_download_file.go

Lines changed: 13 additions & 79 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,6 @@ import (
2626
"github.com/hashicorp/terraform-plugin-framework/resource/schema/stringplanmodifier"
2727
"github.com/hashicorp/terraform-plugin-framework/schema/validator"
2828
"github.com/hashicorp/terraform-plugin-framework/types"
29-
"github.com/hashicorp/terraform-plugin-log/tflog"
3029

3130
"github.com/bpg/terraform-provider-proxmox/fwprovider/attribute"
3231
"github.com/bpg/terraform-provider-proxmox/fwprovider/config"
@@ -74,11 +73,10 @@ func (r sizeRequiresReplaceModifier) PlanModifyInt64(
7473
originalStateSize, err := strconv.ParseInt(string(originalStateSizeBytes), 10, 64)
7574
if err != nil {
7675
resp.Diagnostics.AddError(
77-
"Unexpected error when reading originalStateSize from Private",
78-
fmt.Sprintf(
79-
"Unexpected error in ParseInt: %s",
80-
err.Error(),
81-
),
76+
"Unable to convert original state file size to int64",
77+
"Unexpected error in parsing string to int64, key original_state_size. "+
78+
"Please retry the operation or report this issue to the provider developers.\n\n"+
79+
"Error: "+err.Error(),
8280
)
8381

8482
return
@@ -89,9 +87,10 @@ func (r sizeRequiresReplaceModifier) PlanModifyInt64(
8987
resp.PlanValue = types.Int64Value(originalStateSize)
9088

9189
resp.Diagnostics.AddWarning(
92-
"The file size in datastore has changed.",
90+
"The file size in datastore has changed outside of terraform.",
9391
fmt.Sprintf(
94-
"Previous size %d does not match size from datastore: %d",
92+
"Previous size: %d saved in state does not match current size from datastore: %d. "+
93+
"You can disable this behaviour by using overwrite=false",
9594
originalStateSize,
9695
state.Size.ValueInt64(),
9796
),
@@ -100,53 +99,6 @@ func (r sizeRequiresReplaceModifier) PlanModifyInt64(
10099
return
101100
}
102101
}
103-
104-
urlSizeBytes, diags := req.Private.GetKey(ctx, "url_size")
105-
106-
resp.Diagnostics.Append(diags...)
107-
108-
if (urlSizeBytes != nil) && (plan.URL.ValueString() == state.URL.ValueString()) {
109-
urlSize, err := strconv.ParseInt(string(urlSizeBytes), 10, 64)
110-
if err != nil {
111-
resp.Diagnostics.AddError(
112-
"Unexpected error when reading urlSize from Private",
113-
fmt.Sprintf(
114-
"Unexpected error in ParseInt: %s",
115-
err.Error(),
116-
),
117-
)
118-
119-
return
120-
}
121-
122-
if state.Size.ValueInt64() != urlSize {
123-
if urlSize < 0 {
124-
resp.Diagnostics.AddWarning(
125-
"Could not read the file metadata from URL.",
126-
fmt.Sprintf(
127-
"The remote file at URL %q most likely doesn’t exist or can’t be accessed.\n"+
128-
"To skip the remote file check, set `overwrite` to `false`.",
129-
plan.URL.ValueString(),
130-
),
131-
)
132-
} else {
133-
resp.RequiresReplace = true
134-
resp.PlanValue = types.Int64Value(urlSize)
135-
136-
resp.Diagnostics.AddWarning(
137-
"The file size from url has changed.",
138-
fmt.Sprintf(
139-
"Size %d from url %q does not match size from datastore: %d",
140-
urlSize,
141-
plan.URL.ValueString(),
142-
state.Size.ValueInt64(),
143-
),
144-
)
145-
}
146-
147-
return
148-
}
149-
}
150102
}
151103

152104
func (r sizeRequiresReplaceModifier) Description(_ context.Context) string {
@@ -242,13 +194,12 @@ func (r *downloadFileResource) Schema(
242194
},
243195
},
244196
"size": schema.Int64Attribute{
245-
Description: "The file size.",
197+
Description: "The file size in PVE.",
246198
Optional: false,
247199
Required: false,
248200
Computed: true,
249201
PlanModifiers: []planmodifier.Int64{
250202
int64planmodifier.UseStateForUnknown(),
251-
int64planmodifier.RequiresReplace(),
252203
sizeRequiresReplaceModifier{},
253204
},
254205
},
@@ -284,6 +235,9 @@ func (r *downloadFileResource) Schema(
284235
"specified compression algorithm. Must be one of `gz` | `lzo` | `zst` | `bz2`.",
285236
Optional: true,
286237
Default: nil,
238+
PlanModifiers: []planmodifier.String{
239+
stringplanmodifier.RequiresReplace(),
240+
},
287241
Validators: []validator.String{
288242
stringvalidator.OneOf([]string{
289243
"gz",
@@ -320,9 +274,8 @@ func (r *downloadFileResource) Schema(
320274
Default: booldefault.StaticBool(true),
321275
},
322276
"overwrite": schema.BoolAttribute{
323-
Description: "If `true` and size of uploaded file is different, " +
324-
"than size from `url` Content-Length header, file will be downloaded again. " +
325-
"If `false`, there will be no checks.",
277+
Description: "By default `true`. If `true` and file size has changed in the datastore, " +
278+
"it will be replaced. If `false`, there will be no check.",
326279
Optional: true,
327280
Computed: true,
328281
Default: booldefault.StaticBool(true),
@@ -554,25 +507,6 @@ func (r *downloadFileResource) Read(
554507
return
555508
}
556509

557-
if state.Overwrite.ValueBool() {
558-
// with overwrite, use url to get proper target size
559-
urlMetadata, err := r.getURLMetadata(
560-
ctx,
561-
&state,
562-
)
563-
if err != nil {
564-
tflog.Error(ctx, "Could not get file metadata from url", map[string]interface{}{
565-
"error": err,
566-
"url": state.URL.ValueString(),
567-
})
568-
// force size to -1, which is a special value used in sizeRequiresReplaceModifier
569-
resp.Private.SetKey(ctx, "url_size", []byte("-1"))
570-
} else if urlMetadata.Size != nil {
571-
setValue := []byte(strconv.FormatInt(*urlMetadata.Size, 10))
572-
resp.Private.SetKey(ctx, "url_size", setValue)
573-
}
574-
}
575-
576510
resp.Diagnostics.Append(resp.State.Set(ctx, state)...)
577511
}
578512

‎fwprovider/nodes/resource_download_file_test.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -263,11 +263,12 @@ func uploadIsoFile(t *testing.T, fileName string) {
263263

264264
sshAgent := utils.GetAnyBoolEnv("PROXMOX_VE_SSH_AGENT")
265265
sshUsername := utils.GetAnyStringEnv("PROXMOX_VE_SSH_USERNAME")
266+
sshPassword := utils.GetAnyStringEnv("PROXMOX_VE_SSH_PASSWORD")
266267
sshAgentSocket := utils.GetAnyStringEnv("SSH_AUTH_SOCK", "PROXMOX_VE_SSH_AUTH_SOCK")
267268
sshPrivateKey := utils.GetAnyStringEnv("PROXMOX_VE_SSH_PRIVATE_KEY")
268269
sshPort := utils.GetAnyIntEnv("PROXMOX_VE_ACC_NODE_SSH_PORT")
269270
sshClient, err := ssh.NewClient(
270-
sshUsername, "", sshAgent, sshAgentSocket, sshPrivateKey,
271+
sshUsername, sshPassword, sshAgent, sshAgentSocket, sshPrivateKey,
271272
"", "", "",
272273
&nodeResolver{
273274
node: ssh.ProxmoxNode{

‎fwprovider/test/resource_file_test.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -252,11 +252,12 @@ func uploadSnippetFile(t *testing.T, fileName string) {
252252

253253
sshAgent := utils.GetAnyBoolEnv("PROXMOX_VE_SSH_AGENT")
254254
sshUsername := utils.GetAnyStringEnv("PROXMOX_VE_SSH_USERNAME")
255+
sshPassword := utils.GetAnyStringEnv("PROXMOX_VE_SSH_PASSWORD")
255256
sshAgentSocket := utils.GetAnyStringEnv("SSH_AUTH_SOCK", "PROXMOX_VE_SSH_AUTH_SOCK")
256257
sshPrivateKey := utils.GetAnyStringEnv("PROXMOX_VE_SSH_PRIVATE_KEY")
257258
sshPort := utils.GetAnyIntEnv("PROXMOX_VE_ACC_NODE_SSH_PORT")
258259
sshClient, err := ssh.NewClient(
259-
sshUsername, "", sshAgent, sshAgentSocket, sshPrivateKey,
260+
sshUsername, sshPassword, sshAgent, sshAgentSocket, sshPrivateKey,
260261
"", "", "",
261262
&nodeResolver{
262263
node: ssh.ProxmoxNode{

0 commit comments

Comments
 (0)