Comfy-Org / comfy-cli

Command Line Interface for Managing ComfyUI
https://comfydocs.org/comfy-cli/getting-started
GNU General Public License v3.0
123 stars 15 forks source link

Change tar.gz to zip in saved filename #92

Closed christian-byrne closed 4 weeks ago

christian-byrne commented 1 month ago

Nice work on the CLI and registry progress! Just looked at the code briefly and it's super well organized and very OOP.

Currently, when you publish through the cli, it makes a node.tar.gz file but it's actually a zip file using zlib. Not sure if you had switched from using gzip at some point or maybe some of your other services require that filename, but I changed it to "node.zip" and made that a constant in this PR 👍

robinjhuang commented 1 month ago

Thank you for noticing this! You're right it should be zip. My concern is the existing node packages are already named .tar.gz

@ltdrdata will this change affect your registry implementation?

@yoland68 @hongil0316 anything we need to change for registry-install with this change?

robinjhuang commented 1 month ago

We can also go change all the existing storage bucket file names after this is merged too, but might break older versions

ltdrdata commented 1 month ago

Thank you for noticing this! You're right it should be zip. My concern is the existing node packages are already named .tar.gz

@ltdrdata will this change affect your registry implementation?

@yoland68 @hongil0316 anything we need to change for registry-install with this change?

The file being downloaded is temporary anyway, so the name doesn't matter.

christian-byrne commented 1 month ago

Thank you for noticing this! You're right it should be zip. My concern is the existing node packages are already named .tar.gz @ltdrdata will this change affect your registry implementation? @yoland68 @hongil0316 anything we need to change for registry-install with this change?

The file being downloaded is temporary anyway, so the name doesn't matter.

That's a great point, I get where you're coming from. The zip files were still on my system and I tried opening wtih tar then with 7zip and got errors saying it wasn't tar archived nor was it a gzip. I saw there was a "pack" command in the CLI, so probably worth addressing IMO. I'm thinking you're maybe just speaking to the filesystem on server though?

ltdrdata commented 1 month ago

Thank you for noticing this! You're right it should be zip. My concern is the existing node packages are already named .tar.gz @ltdrdata will this change affect your registry implementation? @yoland68 @hongil0316 anything we need to change for registry-install with this change?

The file being downloaded is temporary anyway, so the name doesn't matter.

That's a great point, I get where you're coming from. The zip files were still on my system and I tried opening wtih tar then with 7zip and got errors saying it wasn't tar archived nor was it a gzip. I saw there was a "pack" command in the CLI, so probably worth addressing IMO. I'm thinking you're maybe just speaking to the filesystem on server though?

The installation feature of the CNR package is still under development. This feature is expected to be supported in a way that integrates with existing custom nodes through cm-cli.

christian-byrne commented 1 month ago

Yeah. Well wrong extension will cause issues with internal scripts, cross-tool interactions, long-term maintenance, etc. It's also a blatant security flag to send a compressed file that obfuscates extension. If the issue is other services relying on the filename, you can switch to using tarfile package from zipfile in the cli program, and keep filename same.

Btw @ltdrdata thanks for all the work you do on the manager - improved so many peoples' lives, you're a legend for that.

ltdrdata commented 1 month ago

Yeah. Well wrong extension will cause issues with internal scripts, cross-tool interactions, long-term maintenance, etc. It's also a blatant security flag to send a compressed file that obfuscates extension. If the issue is other services relying on the filename, you can switch to using tarfile package from zipfile in the cli program, and keep filename same.

Btw @ltdrdata thanks for all the work you do on the manager - improved so many peoples' lives, you're a legend for that.

I believe the validation of compressed files should be handled on the CNR side.

christian-byrne commented 1 month ago

Yeah. Well wrong extension will cause issues with internal scripts, cross-tool interactions, long-term maintenance, etc. It's also a blatant security flag to send a compressed file that obfuscates extension. If the issue is other services relying on the filename, you can switch to using tarfile package from zipfile in the cli program, and keep filename same. Btw @ltdrdata thanks for all the work you do on the manager - improved so many peoples' lives, you're a legend for that.

I believe the validation of compressed files should be handled on the CNR side.

It will be potential/likely security flag for user OS, pipelines, anything that integrates, etc. You disagree with that?

robinjhuang commented 4 weeks ago

If name doesn't matter, I think this PR is good to merge. I can change the names of existing packages to node.zip.