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.

Sign up for GitHub

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

Jump to bottom

#3903 add Background job for cleanup of unneeded remote storages #24095

Closed
wants to merge 1 commit into from
Closed

#3903 add Background job for cleanup of unneeded remote storages #24095

wants to merge 1 commit into from

Conversation

european
Copy link

@european european commented Nov 13, 2020 •
edited by joshtrichards
Loading

Signed-off-by: Nils Werner sammellog@gmail.com

This change add a new background job to execute the command CleanupRemoteStorages

See #3903 for more details.

Fixes #3903

@max-nextcloud
Copy link
Contributor

Looks like this PR was overseen because it has no review was requested.
I'll rebase, resolve the conflicts and request a review.

@max-nextcloud
Copy link
Contributor

@european I hope you don't mind if i squash your commits into one as the follow up commits are all fixing things in the first one.

@max-nextcloud max-nextcloud force-pushed the backgroundjob-remote-storages branch from 93af317 to 0d2b1ee Compare December 14, 2022 15:38
github-advanced-security[bot]
github-advanced-security bot found potential problems Dec 14, 2022
View reviewed changes
apps/files_sharing/lib/BackgroundJob/CleanupRemoteStoragesJob.php
* @param $argument
* @return void
*/
protected function run($argument) {

Check notice

Code scanning / Psalm

MissingParamType

Parameter $argument has no provided type
@max-nextcloud max-nextcloud force-pushed the backgroundjob-remote-storages branch from 0d2b1ee to 9ea3f29 Compare December 14, 2022 15:43
@european
Copy link
Author

@european I hope you don't mind if i squash your commits into one as the follow up commits are all fixing things in the first one.

all fine, I missed this PR as well after that. I was not sure how to continue, since it was my first pr I created for this project

@max-nextcloud max-nextcloud force-pushed the backgroundjob-remote-storages branch from 9ea3f29 to 1868675 Compare January 20, 2023 16:46
@european @max-nextcloud
fix nextcloud#3903: job to cleanup unneeded remote storages
ee33587 
Add a background job for cleanup of unneeded remote storages.

Signed-off-by: european <sammellog@gmail.com>
@max-nextcloud max-nextcloud force-pushed the backgroundjob-remote-storages branch from 1868675 to ee33587 Compare January 20, 2023 16:59
max-nextcloud
max-nextcloud approved these changes Jan 20, 2023
View reviewed changes
Copy link
Contributor

@max-nextcloud max-nextcloud left a comment

Choose a reason for hiding this comment

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

Did not test it but code looks fine.

@max-nextcloud max-nextcloud requested review from a team, ArtificialOwl, icewind1991 and blizzz and removed request for a team January 20, 2023 17:00
@szaimen szaimen added the 3. to review Waiting for reviews label Jan 20, 2023
@szaimen szaimen added this to the Nextcloud 26 milestone Jan 20, 2023
max-nextcloud
max-nextcloud requested changes Jan 22, 2023
View reviewed changes
Copy link
Contributor

@max-nextcloud max-nextcloud left a comment

Choose a reason for hiding this comment

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

Checked in with some colleagues about this pull request.

Instead of triggering the CLI command from the background job the logic should be in a service that is used by both, the Background Job and the CLI command.

That would also avoid using a NullOutput to hide the output.
Background jobs still may need to report errors so hiding the output entirely does not seem like a good idea.

@max-nextcloud
Copy link
Contributor

@european Sorry it took so long to get back to you in the first place.

Would you be willing to pick this up again? I'd be happy to follow up to make sure reviews happen and so on.

@blizzz blizzz modified the milestones: Nextcloud 26, Nextcloud 27 Mar 9, 2023
This was referenced May 3, 2023
Merged
Closed
Merged
@blizzz blizzz mentioned this pull request May 17, 2023
Merged
@blizzz blizzz modified the milestones: Nextcloud 27, Nextcloud 28 May 23, 2023
@skjnldsv skjnldsv mentioned this pull request Nov 1, 2023
Merged
@blizzz blizzz mentioned this pull request Nov 6, 2023
Merged
This was referenced Nov 10, 2023
Merged
Merged
@blizzz blizzz mentioned this pull request Nov 20, 2023
Merged
5 tasks
@blizzz blizzz modified the milestones: Nextcloud 28, Nextcloud 29 Nov 23, 2023
@skjnldsv
Copy link
Member

@european Sorry it took so long to get back to you in the first place.

Would you be willing to pick this up again? I'd be happy to follow up to make sure reviews happen and so on.

I guess not 😢

@european
As there is no feedback since a while I will close this ticket.
If you will decide to work on this feature again and if it hasn't been fixed or implemented already, feel free to re-open and solve the various conflicts.

Thanks for the interest in Nextcloud and the effort put into this! 🙇

@skjnldsv skjnldsv closed this Feb 27, 2024
@skjnldsv skjnldsv removed this from the Nextcloud 29 milestone Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@max-nextcloud max-nextcloud max-nextcloud requested changes

@nickvergessen nickvergessen Awaiting requested review from nickvergessen

@ArtificialOwl ArtificialOwl Awaiting requested review from ArtificialOwl ArtificialOwl was automatically assigned from nextcloud/server-backend

@icewind1991 icewind1991 Awaiting requested review from icewind1991 icewind1991 was automatically assigned from nextcloud/server-backend

@blizzz blizzz Awaiting requested review from blizzz blizzz was automatically assigned from nextcloud/server-backend

Assignees
No one assigned
Labels
enhancement feature: external storage
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Background job for cleanup of unneeded remote storages
5 participants
@european @max-nextcloud @skjnldsv @blizzz @szaimen