-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
fuse travis-ci #2120
fuse travis-ci #2120
Conversation
if err != nil { | ||
return err | ||
if n.PrivateKey == nil { | ||
if err := n.LoadPrivateKey(); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe instead of having this, we should probably just make LoadPrivateKey
not fail if called while the key is already loaded
Yes. I was afraid to break something but that makes sense. Updated and tests are passing locally. |
Working on a rebase for this |
Great thanks!
|
Rebased. I am getting unrelated errors on TestFilePersistence (https://travis-ci.org/thelinuxkid/go-ipfs/jobs/102521435#L252) and TestMultipleDirs (https://travis-ci.org/thelinuxkid/go-ipfs/jobs/102521435#L255). I will try to take a look at them this weekend. |
fab42c9
to
be219bf
Compare
@thelinuxkid the travis-ci err has been logged in #2002. I think this just need a rerun. |
if n.Identity == "" || n.Peerstore == nil { | ||
return errors.New("loaded private key out of order.") | ||
if n.PrivateKey != nil { | ||
return nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
im curious why this had to be added? is there a race condition somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See @whyrusleeping's response to my first comment in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah makes sense thanks
@whyrusleeping and @chriscool could you take a look at these run outputs and confirm all looks good? (i.e. testing fuse as we expect it to be tested, etc): |
My concern with this is that docker is not available so t0300 is not run:
Maybe we could have the tests run in both configurations so that both fuse tests and tests with docker can be run? |
yeah that sounds good. do we know how to set this up? i wish we could pay travis money to add to the number of workers we get. sigh. they keep not letting me pay them!! |
@chriscool we should be able to have two different builders with different env flags. the ipfsbot account should let you experiment with travis, it has perms there. |
@jbenet according to the result of PR #2241, it looks like when "dist: trusty" is set in the .travis.yml file, then docker is not available. So it looks like we might just want to remove "dist: trusty" from .travis.yml, and then we don't need two different builders. One sharness build will test both fuse and docker. @thelinuxkid was there a reason you added, or uncommented, "dist: trusty" into .travis.yml? |
@jbenet fuse was added on trusty build: #2023, travis-ci/travis-ci#1100 (comment) |
FWIT fuse module is already installed on trusty: thelinuxkid@764e756, https://travis-ci.org/thelinuxkid/go-ipfs/builds/104456298. |
@thelinuxkid yeah but it looks like if we don't specify "dist: trusty", we can have both fuse and also docker. |
I am pro containers. Specially if it eliminates the need to depend on travis' build images. But, I know nothing of its support in travis. Having said that, I removed the explicit trusty distribution (https://travis-ci.org/thelinuxkid/go-ipfs/builds/104545197) and travis still used trusty (https://travis-ci.org/thelinuxkid/go-ipfs/jobs/104545198#L14). Is that some sort of confusion...bug...is trusty now the default? |
BTW except for #2214 (which is resolved but needs to be implemented) and stalls, I am not getting any more travis errors. |
@thelinuxkid yeah, it looks like Docker works now on Travis. So now this PR LGTM! Thanks! |
@whyrusleeping or @jbenet could you take a look at merging this? |
This needs a rebase from @thelinuxkid |
Or Anyone can rebase and PR again
|
Rebased. Travis tests failed in a weird way. Rerunning. Will report. |
There is this go test failure:
but it's known and not related. |
@chriscool yep. It's addressed here: #2214. Everything else looks good. |
this looks good to me, mind rebasing on latest master? |
@thelinuxkid do you want me to rebase on master? |
@chriscool just rebased. Thanks. |
Still getting the test failures from #2214. I will try to work on that tomorrow and/or Friday if no one else can pick it up. |
Why did the #2214 happen twice specifically here? |
@rht Because this enables the fuse tests |
I rebased and the travis tests were failing. So I |
Tests passed https://travis-ci.org/thelinuxkid/go-ipfs/builds/98464151.