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: check cid can be fetched from the gateway #15

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

olizilla
Copy link
Member

Check that the newly pinned CID can be fetched from the gateway. Fail the build if not.

fixes #14

License: MIT
Signed-off-by: Oli Evans [email protected]

Check that the newly pinned CID can be fetched from the gateway

fixes #14

License: MIT
Signed-off-by: Oli Evans <[email protected]>
@thattommyhall
Copy link
Member

This is quite defensive, slight worry we might have the opposite problem of builds failing even though nothing is wrong. Perhaps try twice or something?
I suspect someone getting a fail is going to retry the build anyway tho so probably ok, we should make CI more visible to all the peep that change the website too maybe?

@olizilla
Copy link
Member Author

Aye that's a fair point, I don't want all the builds to fail, just the really bad ones. I'll add some retry.

License: MIT
Signed-off-by: Oli Evans <[email protected]>
Comment on lines +71 to +72
# Wait 60s for a response. Wait 10s after a failure. Try the same again, twice.
can_get_from_gateway=$(curl ---max-time 60 --retry 2 --retry-delay 10 --retry-connrefused --fail --silent --show-error "$preview_url") || {
Copy link
Member Author

Choose a reason for hiding this comment

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

@thattommyhall how about dis?

# Attempt to fetch the CID.
# Ignore the response, but fail on error.
# Wait 60s for a response. Wait 10s after a failure. Try the same again, twice.
can_get_from_gateway=$(curl ---max-time 60 --retry 2 --retry-delay 10 --retry-connrefused --fail --silent --show-error "$preview_url") || {

Choose a reason for hiding this comment

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

Wouldn't it make more sense to test whether this CID is available on the cluster ?

Copy link
Member Author

@olizilla olizilla Jan 22, 2021

Choose a reason for hiding this comment

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

Yes! I was going to go that route here, but then parsing the the output of the ipfs-cluster-ctl status command was a little messy and had to make an assumption about the replication factor for the pin.

Adding the ipfs-cluster-ctl add --wait flag will do that job in a more robust way, and I'll add that to this PR if it lands soon, or i'll raise a new one to add it later.

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.

Add check to verify the CID is fetchable before returning it
3 participants