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

List pins and default status value #245

Open
flea89 opened this issue Nov 2, 2022 · 3 comments
Open

List pins and default status value #245

flea89 opened this issue Nov 2, 2022 · 3 comments
Labels
area/compliance check An issue having to do with an existing, or new, pinning compliance check effort/hours exp/novice good first issue Good for newcomers kind/bug P1 status/ready

Comments

@flea89
Copy link
Contributor

flea89 commented Nov 2, 2022

Hi, first of all, thanks for putting this together, it's extremely useful.

I'm fixing some of the reported issues on web3.storage API, but there's one specifically that might be a bit tricky, specifically, the test Can retrieve pin with name 'aCid' via the 'exact' TextMatchingStrategy

The tool sends a

GET https://api.web3.storage/pins?name=1b1c9a55-9abf-4514-8bb2-9d2494d19184&match=exact

and expects some results.

The challenge with that, according to the spec

status 
Array of strings (Status) non-empty
Items Enum: "queued" "pinning" "pinned" "failed"
Example: status=queued,pinning
Return pin objects for pins with the specified status (when missing, service should default to pinned only)

is that if status is not provided should default to "pinned".

I wonder if here we should send a

GET https://api.web3.storage/pins?name=1b1c9a55-9abf-4514-8bb2-9d2494d19184&match=exact&status=queued,pinning,pinned,failed

instead, given we can't assume the service has the given pin request pinned by the time the endpoint is hit by the tool.

What do you think?

Thanks

@olizilla
Copy link
Member

olizilla commented Nov 4, 2022

My gut feel is it's odd that status has a default if omitted, but it makes the simplest curl interaction more approachable.

We should either drop that default, and require that the user send status=pinned to get that behaviour, or we need to be explicit in the spec about the circumstances under which the default status filter applies.

@lidel what you think?

@lidel
Copy link
Member

lidel commented Nov 7, 2022

On default behavior and spec

fwiw the spec is explicit on the behavior here:

(when missing, service should default to pinned only)

iirc this default being pinned instead of "all pins in all states" was suggested by @obo20 to make scaling easier: in their case the "pinned" content being handled by different system than "in progress" pins ("queued,pinning,failed").

I don't mind removing implicit default and requiring explicit status to be always sent to remove any ambiguity, but that should happen in https://github.com/ipfs/pinning-services-api-spec/issues :)

On bug in pinning-service-compliance

For the issue at hand, pinning-service-compliance tests should be fixed to look for any status (send explicit &status=queued,pinning,pinned,failed) when we are testing things that are status-agnostic, such as the match filter.

I was not involved in pinning services for a while, @SgtPooki does this sound good to you?

@lidel lidel added the kind/bug label Nov 7, 2022
@SgtPooki
Copy link
Member

SgtPooki commented Dec 7, 2022

That does sound good. Thanks for chiming in @lidel . Moving this to our work queue

@SgtPooki SgtPooki added P0 P1 area/compliance check An issue having to do with an existing, or new, pinning compliance check effort/hours exp/novice good first issue Good for newcomers status/ready and removed P0 labels Dec 7, 2022
@SgtPooki SgtPooki moved this to Prioritized / Ready for Dev in IPFS-GUI (PL EngRes) Dec 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/compliance check An issue having to do with an existing, or new, pinning compliance check effort/hours exp/novice good first issue Good for newcomers kind/bug P1 status/ready
Projects
None yet
Development

No branches or pull requests

4 participants