feat: add direct source code to clean up empty folders instead of depending on additional plugin

[Pro]

I added a bit more logic to the delete_empty_folder() rule to directly delete empty folders from the script, instead of depending on an additional plugin

[zhan9san]

This feature has been implemented in Artifactory already.

please see comments below.

#37 (comment)

[allburov]

Inline functions are not readable here, it's hard to get the idea from it. Could we move as much as possible outside of the class?
I see there's no self usages here

[Pro]

I will move this to a new utils.py file. It's more readable then.
I would still keep it as inline functions since this is a function very specific to the conversion of list to dict, and not used anywhere else.

[allburov]

It might be either a separate function or class method, but differently not a function inside class method

[Pro]

Did some refactoring now. From my point of view it's now ready to be merged.

I would still argue against putting this as a separate function. It would be out of context. If you still disagree, feel free to direclty update this MR.

[allburov]

Hi, thank you for your time and the contribution!

I like the idea to get rid of the plugin! But have some questions about the code, we should make it more readable and clean.
I don't have Artifactory instance, so can only rely on the code and right now the code does not inspire confidence in me that everything will work :D Let's rearrange it a little

[allburov]

@zhan9san thank you for noticing it!
I think the idea is a bit complicated here, these changes allow us also to exclude files from the folder and THEN decide whether it's an empty folder or not.

As @Pro mentioned in the issue about conan metadata #47

    CleanupPolicy(
       'Delete empty folders',
        rules.repo('conan-testing'),
        rules.delete_empty_folder(),
        # Exclude metadata files. I.e., if a folder only contains these files, consider it as
        # empty
        rules.exclude_filename(['.timestamp', 'index.json']),
    ),

We can't achieve it with Artifactory.

It's tricky, but it should work!

[allburov]

Let's add a note about delete_empty_folder at the end

If you use Artifactory higher then 7.8.1 - it removes empty folders automaticly. We use the rule for some advanced usages, see FAQ section below.

[Pro]

I would leave this up to you to extend this MR with some additional descriptions.

[zhan9san]

@allburov

Understood.

For the Artifactory lower than 7.8.1, they have been EOL.

Version	Release Date	Eol
7.8.1	16-Sep-2020	16-Mar-2022

I prefer to remove the user plugin related function. Maybe adding a waring message first or get feedback from a poll.

Regarding to the special empty dirs like those dirs which only contain '.timestamp', 'index.json' for conan or .npm for npm, how about deleting them recursively like how user plugin does?

If we use AQL here, the list would be too big if the repo contains lots of files.
We can determine whether the dir contains only special files by listdir in artifactory package

[Pro]

Thanks for your comments!

@zhan9san as @allburov mentioned, the Artifactory Built-in Empty Folder deletion would not work here, since the folders still contain metadata files.

I see two solutions to this metadata leftover issue:

1. A new set of rules is needed, only for conan (similar to the docker rules). These rules could then check the specific conan package files and delete the complete conan package, including metadata. The problem I see with this solution is that it may get quite complext, and especially considering the upcoming Conan 2 release, the folder structure may change. So these rules would need to be constantly updated

For these specific rules we could then implement the recusrive deletion as proposed by @zhan9san

2. Use the folder deletion as proposed in this MR. Downside is, that the "delete older than" rule could also only partially delete files from a valid conan package and therefore invalidate it.

@allburov I can work on your comments. I would also like to add a few unit tests for the methods, especially the filter.
Can you extend your github actions to also run pytest?

[Pro]

I prefer to remove the user plugin related function. Maybe adding a waring message first or get feedback from a poll.

I would just simply remove it. With semantic versioning and 0.x versions, a change from 0.3 to 0.4 means breaking change. If one really needs the plugin, he can stick with the old version.

[allburov]

I prefer to remove the user plugin related function.
I would just simply remove it.

Let's do it then with a record in changelod.md about it!
We can do it in a separate PR if we find another solution for Conan repositories.

@Pro

So these rules would need to be constantly updated
The workaround with combing delete_empty_folder + exclude_filename also needs to have support as well.

Is the set of metadata files well defined for Conan? If it does and from path to conan_package.tgz there's a way to find paths to metadata files - we can extend delete_older_than a little this way:

class ConanMixin:
    """ Also removes all metatada files for conan packages v1 """
    index_files = ['.timestamp', 'index.json']

    def _aql_add_filter(aql_query_list):
        aql = super().__aql_add_filter(aql_query_list)
        exclude_filename(self.index_files)._aql_add_filter(aql_query_list)
        return aql

    def _filter_result(self, result_artifact):
        for conan_package_path in result_artifact:
              index_path = ... # build path to some index
             other_metadata_path = ... # build other paths
             result_artifact.append(index_path)
             result_artifact.append(other_metadata_path)
        return result_artifact
    
class conan_delete_older_than(ConanMixin, delete_older_than):
    pass

class conan_delete_not_used_since(ConanMixin, delete_not_used_since):
   pass

It doesn't look to hard to support it.
I'd suggest to add rules/conan.py and add mixin and necessary classes there

[allburov]

Can you extend your github actions to also run pytest?

I thought we're running them already... Feel free to create a PR for it, not a problem!
I think forks also run Github Actions - you can use master branch for it in your repo to test it

[Pro]

Can you extend your github actions to also run pytest?

I thought we're running them already... Feel free to create a PR for it, not a problem! I think forks also run Github Actions - you can use master branch for it in your repo to test it

I added now an additional Github action. See #51

Would be good if you can merge that first, then I will include it in this MR

[Pro]

Is the set of metadata files well defined for Conan? If it does and from path to conan_package.tgz there's a way to find paths to metadata files - we can extend delete_older_than a little this way:

Unfortunately it's not that easy. It depends a bit on your conan settings (i.e., package references enabled/disabled).

Here it is enabled:

And Here disabled:

You can see the additional level of folders in the first one. And the cleanup may only also delete a subset of specific packages.
For Conan 2, the folder structure may also change.

Not sure though. Better to move the discussion to a new issue/MR. I will stick for now with the solution proposed in this MR.

[allburov]

@Pro yep, let's add delete_empty_folder then as you suggested

[Pro]

@allburov I did a refactoring.

From my side I would see this MR ready to be merged.
Unfortunately, I cannot work more extensively on this. If you prefer some additional changes, feel free to directly update this MR :)

[allburov]

@Pro I'll look at this when I have free time and will ask to test the code because I don't have an Artifactory instance where I can test it.
Thanks for providing tests, it'll help!

[allburov]

Could you look and test changes from #52?

[allburov]

closed in favor #52