KeepLatestVersionNFilesInFolder is not working as expected

[seth-stvns]

Problem Explanation

I have been looking into the KeepLatestVersionNFilesInFolder to clean up our docker images as we want to keep N highest versions. I think I found an issue with this rule in the filter function. I tested this with both a docker registry and maven repository in artifactory.

What I am seeing is that when the regex is set to something that will find my version numbers (default won't work for me), is that the run explodes with the following error --

Traceback (most recent call last):
  File "/usr/local/bin/artifactory-cleanup", line 8, in <module>
    sys.exit(ArtifactoryCleanupCLI())
  File "/usr/local/lib/python3.9/site-packages/plumbum/cli/application.py", line 178, in __new__
    return cls.run()
  File "/usr/local/lib/python3.9/site-packages/plumbum/cli/application.py", line 624, in run
    retcode = inst.main(*tailargs)
  File "/usr/local/lib/python3.9/site-packages/artifactory_cleanup/cli.py", line 121, in main
    for summary in cleanup.cleanup(
  File "/usr/local/lib/python3.9/site-packages/artifactory_cleanup/artifactorycleanup.py", line 53, in cleanup
    artifacts_to_remove = policy.filter(artifacts)
  File "/usr/local/lib/python3.9/site-packages/artifactory_cleanup/rules/base.py", line 284, in filter
    artifacts = rule.filter(artifacts)
  File "/usr/local/lib/python3.9/site-packages/artifactory_cleanup/rules/keep.py", line 181, in filter
    artifacts.keep(good_artifacts)
  File "/usr/local/lib/python3.9/site-packages/artifactory_cleanup/rules/base.py", line 32, in keep
    return self.remove(artifacts)
  File "/usr/local/lib/python3.9/site-packages/artifactory_cleanup/rules/base.py", line 42, in remove
    print(f"Filter package {artifact['path']}/{artifact['name']}")
TypeError: list indices must be integers or slices, not str

Looks like a value being sent to keep from filter is not matching the expected object type.

Debugging

Looking at the code artifactory_cleanup/rules/keep.py#L140 I see that good_artifacts is being used as an argument for artifacts.keep.

Looking into how that variables value gets set it comes from artifacts_by_path_and_name.values(). Which is appended to artifacts_by_path_and_name[key].append(artifactory_with_version) in the for loop over the artifacts provided to the filter. That is where I noticed that artifactory_with_version = [version_str, artifact] is an array with a string and dictionary values.

That means that artifacts_by_path_and_name.values() returns an array of arrays with values of ['String', 'Dict']. For example --

[ [ 'Version', { ArtifactDict } ], [ 'Version', { ArtifactDict } ] ]

Based on what I can tell the keep function is expecting to take in either an Artifact Dictionary i.e. {ArtifactDict} or an array of Artifact Dictionaries i.e. [ { ArtifactDict } ]. So passing it an array like the one above causes it to error out.

Potential fix

I believe fixed by pulling the Artifact Dictionary out of the result of artifactory_with_version[good_artifact_count:] and then calling keep with that value.

Something like --

# Replacement for https://github.com/devopshq/artifactory-cleanup/blob/master/artifactory_cleanup/rules/keep.py#L180
            good_sets = artifactory_with_version[good_artifact_count:]
  
            good_artifacts = []
  
            for good_set in good_sets:
              good_artifacts.append(good_set[1])

I am more than happy to put in a PR but wanted to submit an issue here first to see if I am missing something before doing so.

[allburov]

It'd be great if one create PR with the fix.

I think we should start create tests for each rules - the reference can be found here https://github.com/devopshq/artifactory-cleanup/blob/master/tests/test_rules_keep.py

[seth-stvns]

Sure can do! Will submit a PR today or tomorrow.

[seth-stvns]

@allburov I don't see a contribution section or agreement, am I just missing it or is this project not using one?

[allburov]

@seth-stvns yes, there's no agreement yet.
Just clone the repo and start changing it!

Feel free to ask any question here anyway! I'll create a guide a bit later based on your questions if you have any

[seth-stvns]

@allburov opened PR #82 to fix this issue. Please let me know if you have any questions or concerns.

[allburov]

I like this type of developer - opened the issue himself, fixed the issue himself - you make open-source world so great! ❤️
@seth-stvns thank you!

[allburov]

Fixed in 1.0.3

[seth-stvns]

Thank you for the help and review! Happy to help make this project better. It is a very helpful tool.