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

feat: adds new flags to tootctl media remove #1154

Closed
wants to merge 1 commit into from
Closed

feat: adds new flags to tootctl media remove #1154

wants to merge 1 commit into from

Conversation

quicoto
Copy link
Contributor

@quicoto quicoto commented Jan 31, 2023

Adds documentation from this PR https://github.com/mastodon/mastodon/pull/22149/files landing in the next Mastodon stable 4.1.0

@vercel
Copy link

vercel bot commented Jan 31, 2023

@quicoto is attempting to deploy a commit to the Mastodon Team on Vercel.

A member of the Team first needs to authorize it.

@quicoto
Copy link
Contributor Author

quicoto commented Jan 31, 2023

@evanphilip could you review? Mostly used your existing commented from the PR

Copy link

@evanphilip evanphilip left a comment

Choose a reason for hiding this comment

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

Perhaps we should also update the description of tootctl media remove to something like:

Removes locally cached copies of media attachments, avatars or profile headers from other servers. By default, only media attachments are removed.

@evanphilip
Copy link

evanphilip commented Feb 1, 2023

Thanks for taking this up! I realize that the options are not very elegant the way they are, but I did not want to break backward-compatibility of tootctl media remove. As it is now, tootctl media remove still does what it used to do unless you include one of the new options.

@evanphilip
Copy link

@quicoto is attempting to deploy a commit to the Mastodon Team on Vercel.

A member of the Team first needs to authorize it.

@quicoto I am not part of the Mastodon Team on Vercel and the switch seems recent. However, the failed test doesn't seem to block merging as seen here: https://github.com/mastodon/documentation/pulls?q=is%3Apr+is%3Aclosed+is%3Amerged

@quicoto
Copy link
Contributor Author

quicoto commented Feb 1, 2023

Removes locally cached copies of media attachments, avatars or profile headers from other servers. By default, only media attachments are removed.

Done, commit amended.

@quicoto I am not part of the Mastodon Team on Vercel and the switch seems recent. However, the failed test doesn't seem to block merging as seen here: https://github.com/mastodon/documentation/pulls?q=is%3Apr+is%3Aclosed+is%3Amerged

No worries, first I wanted you to approve this as you know first hand how it works. Then we find the correct reviewer :)

Adds documentation from PR 22149
@ClearlyClaire
Copy link
Contributor

Note that I approved the PR, but I am not able to merge it. As far as I know, only @Gargron is currently able to merge documentation changes.

@quicoto quicoto closed this by deleting the head repository Feb 9, 2023
@quicoto quicoto reopened this Feb 10, 2023
@quicoto
Copy link
Contributor Author

quicoto commented Feb 10, 2023

@Gargron Since 4.1.0 was just released, can we merge this PR?

Thank you

@sznowicki
Copy link

@quicoto thanks for this PR. I was looking to get some info on how to remove that 19GB of "headers" I get on my private instance and this works like a charm. Also kudos to that previous PR author (@evanphilip) who implemented that feature ❤️

@marians
Copy link

marians commented Feb 14, 2023

I suggest to add a bit of explanation on the new options, especially

  • What does the --prune-profiles flag really do? What does prune mean here? This word doesn't appear in the other flags. So users should not be left wondering how this flag is different from the others.

  • Which flags can be combined, which one exclude each other?

@quicoto
Copy link
Contributor Author

quicoto commented Feb 14, 2023

Hey @marians thanks for the feedback.

My PR here is mostly using what's in the core https://github.com/mastodon/mastodon/pull/22149/files

I'd love to see this merged, to at least have something to iterate over. 4.1.0 is out and the docs are out of date, but nobody seems to have permission to merge this.

@evanphilip
Copy link

evanphilip commented Feb 15, 2023

Thank you @marians for the feedback! It was my responsibility to update the documentation while pushing the new flag and I am really grateful to @quicoto for taking care of it!

  1. (transitive, figuratively) To cut down or shorten (by the removal of unnecessary material).
    to prune a budget, or an essay
  • I agree that the current syntax for this feature is very clunky, but it is a bit late to change it easily. The next best thing is to document it clearly.
  • @marians What do you think about updating the documentation to this? It is a bit verbose. Suggestions are welcome!

--days N: How old media attachments have to be before they are removed. In case of avatars and headers, how old
the last webfinger request and update to the user has to be before they are removed. Defaults to 7.
--prune-profiles: Instead of media attachments, remove locally cached copies of avatars and headers from other servers. Cannot be combined with --remove-headers.
--remove-headers: Instead of media attachments, remove locally cached copies of headers from other servers. Cannot be combined with --prune-profiles.
--include-follows: Override the default behavior of --prune-profiles and --remove-headers to remove locally cached copies of avatars (and headers) from other servers, irrespective of follow status (by default, they are only removed from accounts that are not followed by or following anyone locally). Can only be used with --prune-profiles or --remove-headers.

@marians
Copy link

marians commented Feb 15, 2023

Great improvement @evanphilip !

@quicoto
Copy link
Contributor Author

quicoto commented Feb 15, 2023

What a mess, I did delete my forked repo a few days ago and now I can't seem to be able to update this PR 🤦 I might have to create a new PR with the latest suggestion from Evan

@quicoto
Copy link
Contributor Author

quicoto commented Feb 15, 2023

Closing this PR in favor of the updated here:

#1172

Thank you.

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

Successfully merging this pull request may close these issues.

5 participants