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

add threads flag to use threads #132

Merged
merged 4 commits into from
Apr 2, 2024
Merged

Conversation

Rommmmm
Copy link
Contributor

@Rommmmm Rommmmm commented Feb 19, 2024

also add missing readme for ignore-not-found

@Rommmmm
Copy link
Contributor Author

Rommmmm commented Feb 19, 2024

@allburov this one is for threads

README.md Outdated Show resolved Hide resolved
@Rommmmm
Copy link
Contributor Author

Rommmmm commented Feb 20, 2024

@allburov fixed everything, please let me know if my way of putting the context inside the thread is fine with you

@@ -56,9 +59,15 @@ def cleanup(self, block_ctx_mgr, test_ctx_mgr) -> Iterator[CleanupSummary]:
print(f"Found {len(artifacts_to_remove)} artifacts AFTER filtering")

# Delete artifacts
for artifact in artifacts_to_remove:
def _delete_with_context(artifact, policy, test_ctx_mgr, get_name_for_ci, destroy, ignore_not_found):
Copy link
Member

@allburov allburov Feb 20, 2024

Choose a reason for hiding this comment

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

only artifact argument is required here as I can see, so the signature should be something like
All other methods are imported, flags we can get from self as well.

def _delete(artifact):
    with test_ctx_mgr(get_name_for_ci(artifact)):
        policy.delete(artifact, destroy=self.destroy)

executor.submit(self._delete, artifact=artifact)

Also we could switch to https://docs.python.org/3/library/concurrent.futures.html#concurrent.futures.Executor.map

executor.map(self._delete, artifacts_to_remove)

P.S.
Well, it'll create mess in output anyway, because all thread will print in the same output... But it's the useful case for people who doesn't care about CI, agree.
The task is not so easy if we think about that, especially in case where you use it in CI.

@gruselglatz
Copy link

gruselglatz commented Mar 15, 2024

Multihreading would be awesome for lager repos. looking forward to it :)

@Rommmmm
Copy link
Contributor Author

Rommmmm commented Mar 15, 2024

Im sorry i was super busy i try to fix the PR on Sunday

@Rommmmm
Copy link
Contributor Author

Rommmmm commented Apr 1, 2024

@allburov done with your last comment, sorry it took so long :\

Copy link
Member

@allburov allburov left a comment

Choose a reason for hiding this comment

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

One small thing and we're good to go!
Could you also test it works with no issue without worker-count (as it's right now, aka as 1) and with some worker-count, like 4?
I don't have artifactory to test that one :(

artifactory_cleanup/artifactorycleanup.py Outdated Show resolved Hide resolved
@Rommmmm
Copy link
Contributor Author

Rommmmm commented Apr 2, 2024

@allburov i did test it with artifactory, im running my clusters retention with 3 workers daily already for a week and its running without issues

@allburov allburov merged commit e2c4a0d into devopshq:master Apr 2, 2024
2 checks passed
@allburov
Copy link
Member

allburov commented Apr 2, 2024

@Rommmmm thank you! 👏

zkygr pushed a commit to zkygr/artifactory-cleanup that referenced this pull request Jul 10, 2024
* add threads flag to use threads and add missing readme for ignore-not-found

* add test context manager inside thread

* review

* review
zkygr pushed a commit to zkygr/artifactory-cleanup that referenced this pull request Jul 10, 2024
* add threads flag to use threads and add missing readme for ignore-not-found

* add test context manager inside thread

* review

* review
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants