feat: ipfs version flags + ipfs repo version
#1181
Conversation
…pfs.version should report the repo version so I might implement it there. Still need the /repo/version http api, though.
…ags, partial implementation of --commit (prints empty string for now). Not sure how to get the commit info, go-ipfs doesn't seem to implement it either.
…rsion to validate the test.
…api repo/version.
|
CI is failing because this change requires the How do y'all usually get around this? |
|
Thanks for contributing and welcome to the project! Let's get the one in js-ipfs-api merged first and then come back to this one :) |
|
@JonKrone js-ipfs-api was released with your patch, you can update it as a dep of this PR so that tests pass :) |
|
Looks like there's a failing test related to interface-ipfs-core tests of pubsub (https://travis-ci.org/ipfs/js-ipfs/jobs/332979384#L4315) and a set of fails from I don't quite have time to look into sharness today though it looks like updating expected output. I noticed in #1103 that the |
|
@JonKrone merging into a branch on the main repo so that I can fix sharding :) |
| version: pkg.version, | ||
| repo: repoVersion, | ||
| commit: '' | ||
| }) |
There was a problem hiding this comment.
@JonKrone found the issue. Here you are assuming that IPFS has always been init when it is not the case always. the Sharness test is checking the version with ipfs offline and without init.
There was a problem hiding this comment.
Thanks! In order to avoid the init constraint, go-ipfs accesses the repo version directly from a constant exposed by go-ipfs/repo so the repo version is available offline and without an initialized repo. Analogous to const repoVersion = require('ipfs-repo').version
Unfortunately, there's no similar field exposed by the js-ipfs-repo package. We could read the js-ipfs-repo/constants file directly like we're doing with pkg.version above or add the version as an export of the js-ipfs-repo package.
If we need to be able to read the version w/o init, I think an export is the way to go and replace the dependence of core/components/repo.version on an initialized _repo with just require('ipfs-repo').version. Should we then add this version constant to the repo SPEC?
Feels like a big change to not use the repo API, what are your thoughts?
There was a problem hiding this comment.
We can totally expose the constant, however, I do not think is the best approach. If we use the version from the module, then we might have a false positive where the module is signaling the latest repo version than the one that is inited.
I feel that the right way to do it is:
- if repo inited, then read the
versionfrom the repo - if the repo is not inited, then read the
versionfrom the constant
There was a problem hiding this comment.
Cool, I agree! I looked into what happens when the version is outdated and found that starting the daemon with an outdated ~./jsipfs/version throws during startup. So even running js-ipfs --help fails. test: echo "5" > ~/.jsipfs/version && jsipfs --help.
If that's expected behavior, I can't think of a case where you can run a cli command on an initialized but outdated repo, and we could safely report the constant version.
I think you have the right of it, though, and we should aim for reporting the version of ~/.jsipfs/version if the repo is initialized and maybe at some point in the future, change startup so that we delay opening the repo connection or make accessing the version independent of opening a repo?
either way: ipfs/js-ipfs-repo#158
Hi all,
First time looking at any of the ipfs codebases, wanted to contribute a little thing so I could feel my way around it.
api/v0/repo/versionhttp apiipfs repo version--number,--repo, and--allflags for theipfs versioncommand--commitflag toipfs versionrelated js-ipfs-api PR
Modeled
ipfs versionbehavior off of go-ipfs.The
--commitflag is parsed but doesn't do much because I don't know how to get the commit hash of the current ipfs version. In go-ipfs, it's made available at project build-time but I didn't see something like that for us. Don't think implementing it is necessary for this PR.