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: support optional pin names #10261

Merged
merged 7 commits into from
Jan 4, 2024
Merged

feat: support optional pin names #10261

merged 7 commits into from
Jan 4, 2024

Conversation

hacdias
Copy link
Member

@hacdias hacdias commented Dec 15, 2023

Closes #4586. Requires ipfs/boxo#525.

A name will apply to all CIDs being pinned. So if you ipfs pin add --label A [CID1] [CID2] [...], all CIDs will have label "A".

@hacdias hacdias self-assigned this Dec 15, 2023
@hacdias hacdias requested a review from a team as a code owner December 15, 2023 09:33
@hacdias hacdias marked this pull request as draft December 15, 2023 09:36
core/commands/pin/pin.go Outdated Show resolved Hide resolved
core/commands/pin/pin.go Outdated Show resolved Hide resolved
@hacdias hacdias force-pushed the pin-labels branch 2 times, most recently from 79c90b7 to fec88d8 Compare January 2, 2024 11:33
@hacdias hacdias requested a review from lidel January 2, 2024 15:44
@lidel lidel changed the title feat: add support for optional pin labels feat: support optional pin names Jan 2, 2024
@hacdias hacdias marked this pull request as ready for review January 3, 2024 13:29
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

@hacdias thank you, I've tested locally, added JSON tests (/api/v0 wire format and --enc=json) and some --help text.

I think this is good to go, as long you address cosmetics:

Comment on lines 72 to 74
- Pin add is idempotent: pinning CID which is already pinned won't change
the name, value passed with '--name' with the original pin is preserved.
To rename pin, use 'pin rm' and 'pin add --name'.
Copy link
Member

@lidel lidel Jan 4, 2024

Choose a reason for hiding this comment

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

@hacdias I've added this because running ipfs pin add --name A CID and then ipfs pin add --name B CID does not update the name (it is still A).

If you feel we could get follow-up calls update the name withoiut huge refactor, that would be preferable, and we could remove this awkward caveat sentence.

docs/changelogs/v0.26.md Outdated Show resolved Hide resolved
@hacdias
Copy link
Member Author

hacdias commented Jan 4, 2024

@lidel I updated to --names. Regarding updating the label: I think that can be done solely on Boxo's side but it's more involved. I will let that for a separate PR (will open issue).

@hacdias hacdias merged commit a8a6bbe into master Jan 4, 2024
15 of 23 checks passed
@hacdias hacdias deleted the pin-labels branch January 4, 2024 13:25
@lidel lidel mentioned this pull request Jan 4, 2024
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Support optional labels for local pins
2 participants