containerbuildsystem / cachi2

GNU General Public License v3.0
7 stars 25 forks source link

interface: cli: Remove the top-level output directory before fetching #509

Closed eskultety closed 4 months ago

eskultety commented 5 months ago

We keep reusing the old output directory hierarchy with repeated fetch runs, overwriting all relevant files. Right now it's all fine because all the files have predictable (pretty much fixed) names, however if we create files/directories with any randomness in their names, we'll be just creating more and more noise with repeated runs. Since we're rewriting all files for the processed package managers anyway (for now), i.e. not reusing data in between runs, it would be better if we first cleaned up the destination if it exists and only then proceed with the fetch.

Note that failing to process a request would be the preferred approach if the output directory existed, because we can't be sure that the existing directory we're being pointed to was indeed created by us so we may end up deleting other legitimate data, however, given that such logic would lead to regressive behaviour since we already happily overwrite anything the user points us at, so the new behaviour isn't any worse than it already is.

Maintainers will complete the following section

Note: if the contribution is external (not from an organization member), the CI pipeline will not run automatically. After verifying that the CI is safe to run:

ben-alkov commented 5 months ago

*faints* all that work I did on checksumming existing pypi artifacts instead of re-downloading... :smiling_face_with_tear:

ben-alkov commented 5 months ago

LGTM, but I don't want to ACK when we still have concerns about possibly deleting important user directories (in the case where a user points --output at something they might regret).

eskultety commented 5 months ago

LGTM, but I don't want to ACK when we still have concerns about possibly deleting important user directories (in the case where a user points --output at something they might regret).

Well, as discussed we can only delete either the deps or deps/<pkg_manager> directories instead for now if everyone is in agreement.

brunoapimentel commented 5 months ago

LGTM, but I think we can add unit tests just to get coverage of the new functionality. I went ahead and wrote a basic unit test for this, you can use it if you think it's good enough.

In any case, I will go ahead and leave my LGTM so you can merge this as soon as you're back.

eskultety commented 4 months ago

LGTM, but I think we can add unit tests just to get coverage of the new functionality. I went ahead and wrote a basic unit test for this, you can use it if you think it's good enough.

In any case, I will go ahead and leave my LGTM so you can merge this as soon as you're back.

Applied, thanks!