Skip to content

Draft pull request for code review#1

Draft
mallsbill wants to merge 11 commits intotofee/kdrivefrom
infomaniak/kdrive
Draft

Draft pull request for code review#1
mallsbill wants to merge 11 commits intotofee/kdrivefrom
infomaniak/kdrive

Conversation

@mallsbill
Copy link

@mallsbill mallsbill commented Feb 17, 2026

  • add multipart upload
  • internal tests
  • create shared link


type SessionCancelResponse struct {
Result string `json:"result"`
Data Item `json:"data"`
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: wrong type

func (u *uploadSession) Abort(ctx context.Context) error {
opts := rest.Opts{
Method: "POST",
Path: fmt.Sprintf("/3/drive/%s/upload/session/%s/cancel", u.f.opt.DriveID, u.token),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: this route doesn't exist

Comment on lines -61 to -66
}, {
Name: "account_id",
Help: `Fill the account ID that is to be considered for this kdrive.
When showing a folder on kdrive, you can find the account_id here:
https://ksuite.infomaniak.com/{account_id}/kdrive/app/drive/...`,
Default: "",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: need to delete this from the doc example as well

return err
}

// purgeCheck removes the root directory, if check is set then it
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: I guess root should be given ?


// TODO: check if the following erroneous behavior also happens on kDrive
// (this workaround comes from the pcloud backend implementation)
if err != nil {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: please check this

Comment on lines 57 to 62
Name: "root_folder_id",
Help: "Fill in for rclone to use a non root folder as its starting point.",
// for default root, see https://developer.infomaniak.com/docs/api/get/3/drive/%7Bdrive_id%7D/files/%7Bfile_id%7D
Default: "1",
Default: "5",
Advanced: true,
Sensitive: true,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought: I feel like this only makes the backend harder to use. Users shouldn't have to worry about the real root of their drive.

This should be hidden/automated.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From internal discussion:

  • /1 : real root (possible, discouraged)
  • private: commodity for private root, ID resolved at runtime (default)
  • common: commodity for common root, ID resolved at runtime
  • <id>: explicit folder ID

Comment on lines 70 to 72
Name: "access_token",
Help: `Access token generated in Infomaniak profile manager.`,
Default: "",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: this has to be set to Sensitive: true

Comment on lines 57 to 62
Name: "root_folder_id",
Help: "Fill in for rclone to use a non root folder as its starting point.",
// for default root, see https://developer.infomaniak.com/docs/api/get/3/drive/%7Bdrive_id%7D/files/%7Bfile_id%7D
Default: "1",
Default: "5",
Advanced: true,
Sensitive: true,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From internal discussion:

  • /1 : real root (possible, discouraged)
  • private: commodity for private root, ID resolved at runtime (default)
  • common: commodity for common root, ID resolved at runtime
  • <id>: explicit folder ID

f.dirCache.Put(remote, strconv.Itoa(info.ID))

d := fs.NewDir(remote, info.ModTime()).SetID(strconv.Itoa(info.ID))
// FIXME more info from dir?
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: please check this

Comment on lines 1063 to 1067
// FIXME
// if apiErr.Code == "not_found" || apiErr.Code == "trashed" {
// return fs.ErrorObjectNotFound
// }
//}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: please check this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Comments