Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Web UI: Upload to tmp file name then rename if successful #1272

Merged
merged 2 commits into from
Oct 31, 2023

Conversation

rdmark
Copy link
Member

@rdmark rdmark commented Oct 28, 2023

Additionally, in order to simplify access to the file commands class in the file upload function, refactoring associated code to:

  1. Move the dropzone.js operations back into web.py
  2. Move list_images() from file commands into piscsi commands (it was the only class method in that package that calls the protobuf interface)

@rdmark rdmark force-pushed the rdmark-issue-1270 branch 2 times, most recently from b354733 to 70c7184 Compare October 28, 2023 11:13
@rdmark rdmark requested a review from nucleogenic October 28, 2023 11:29
@rdmark rdmark force-pushed the rdmark-issue-1270 branch from 70c7184 to c9468a1 Compare October 28, 2023 11:34
@rdmark rdmark changed the title Upload to tmp file name then rename if successful Web UI: Upload to tmp file name then rename if successful Oct 29, 2023
try:
archive_info = self._get_archive_info(
f"{server_info['image_dir']}/{file.name}",
_cache_extra_key=file.size,
Copy link
Member

Choose a reason for hiding this comment

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

I think I remember what this is for now, which was a somewhat brittle cache invalidator for when a file is replaced with another of the same name. Otherwise, we'll return the previous file.

Copy link
Member Author

Choose a reason for hiding this comment

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

Aha! I was confused because _cache_extra_key wasn't actually called in our code. So I assume it's used by the cache library behind the scenes then?

Copy link
Member Author

Choose a reason for hiding this comment

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

Restored those two lines

@rdmark rdmark force-pushed the rdmark-issue-1270 branch from c9468a1 to bed15f6 Compare October 30, 2023 00:02
@@ -41,18 +37,8 @@ class FileCmds:
class for methods reading from and writing to the file system
"""

def __init__(self, sock_cmd: SocketCmds, piscsi: PiscsiCmds, token=None, locale=None):
self.sock_cmd = sock_cmd
def __init__(self, piscsi: PiscsiCmds):
Copy link
Member Author

Choose a reason for hiding this comment

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

@nucleogenic Question! What do you think about not initializing the class with the PiscsiCmds object? The original motivation was for sharing the token and locale, I believe. But now when FileCmds doesn't access the protobuf interface anymore it's not really needed, is it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Never mind; I gave it a go and quickly realized that instantiating a new PiscsiCmds object is a pain.

@rdmark rdmark merged commit 029cf06 into develop Oct 31, 2023
6 checks passed
@rdmark rdmark deleted the rdmark-issue-1270 branch October 31, 2023 21:54
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