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: warn user when 'pin remote add' while offline #8621

Merged

Conversation

rukolahasser
Copy link
Contributor

@rukolahasser rukolahasser commented Dec 20, 2021

Resolves #8418

The warning is printed only when being online matters (if CID to be pinned remotely is in a local store)

@welcome

This comment has been minimized.

Copy link
Contributor

@schomatis schomatis left a comment

Choose a reason for hiding this comment

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

This seems be the correct way. My main ask at the moment would be to include a test for it in sharness (t0700-remotepin.sh), although I'm not sure how easy it is to deal with the situation that the command will still hang forever even after the warning (maybe we should add a timeout).

@rukolahasser
Copy link
Contributor Author

@schomatis How do I set up the TEST_DOCKER_HOST? I tried running ipfs in docker locally and set the value to localhost, but it failed. Was I missing anything?

@schomatis
Copy link
Contributor

Could you give me more context, please? What are you trying to do and why do you need TEST_DOCKER_HOST?

@rukolahasser
Copy link
Contributor Author

This seems be the correct way. My main ask at the moment would be to include a test for it in sharness (t0700-remotepin.sh), although I'm not sure how easy it is to deal with the situation that the command will still hang forever even after the warning (maybe we should add a timeout).

I was trying to write a test for it as you mentioned above, and bootstrapped a rb-pinning-service-api with docker but the test still failed to run.

@schomatis
Copy link
Contributor

I'm still not following completely without seeing the code but the idea is to put the test in sharness (in the file cited before), so Docker shouldn't be necessary for that.

@rukolahasser
Copy link
Contributor Author

Turned out that I didn't have jq installed on my machine, now it's fixed. I've added a test and updated the PR, could you have a look please?

@rukolahasser
Copy link
Contributor Author

Does everything look good in the PR? @schomatis

@schomatis
Copy link
Contributor

schomatis commented Jan 7, 2022

Thanks for the ping, yes. Still we need to wait for lidel to confirm on #8418 (comment) to make sure if there is anything else needed.

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.

LGTM, thank you.

@lidel
Copy link
Member

lidel commented Jan 19, 2022

@rukolahasser mind rebasing this PR on top of the latest master ? It should fix the interop tests (we need CI to be green before we merge this)

@rukolahasser rukolahasser force-pushed the feat/cmds/add-warning-while-offline branch from 7e0ece9 to 8d2af0d Compare January 20, 2022 15:38
@rukolahasser
Copy link
Contributor Author

@lidel I rebased it but there was a failed test that seemed unrelated to my PR, was the failed test expected?

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.

Thank you, this LGTM. (Pushed small fix to ensure output of --enc=json is not impacted)

CI issue was unrelated, all green now.

This ensures we don't break JSON produced by --enc=json
@lidel lidel force-pushed the feat/cmds/add-warning-while-offline branch from 459d533 to f29b38b Compare January 20, 2022 21:43
@guseggert guseggert merged commit e93d6fb into ipfs:master Feb 15, 2022
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.

Warn user when 'pin remote add' while offline
4 participants