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.

Sign up for GitHub

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

Jump to bottom

feat(daemon): print version #5503

Merged
merged 1 commit into from
Sep 21, 2018
Merged

feat(daemon): print version #5503

merged 1 commit into from
Sep 21, 2018

Conversation

overbool
Copy link
Contributor

@overbool overbool commented Sep 20, 2018 •
edited
Loading

License: MIT
Signed-off-by: Overbool overbool.xu@gmail.com

Fixes: #5498

kevina
kevina suggested changes Sep 21, 2018
View reviewed changes
Copy link
Contributor

@kevina kevina left a comment

Choose a reason for hiding this comment

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

See comment.

It should also likely have a sharness test case.

cmd/ipfs/daemon.go Outdated
@@ -191,6 +193,11 @@ func daemonFunc(req *cmds.Request, re cmds.ResponseEmitter, env cmds.Environment
log.Errorf("Injecting prometheus handler for metrics failed with message: %s\n", err.Error())
}

// print the ipfs version
fmt.Printf("go-ipfs version: %s\n"+
"Repo version: %d\nSystem version: %s\nGolang version: %s\n",
Copy link
Contributor

Choose a reason for hiding this comment

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

I would use separate Printf for each line.

If not than at least be consistent and remove the "+" operator and put everything on one line.

@overbool
Copy link
Contributor Author

overbool commented Sep 21, 2018 •
edited
Loading

@Stebalien @kevina

make install will call go install -ldflags="-X "github.com/ipfs/go-ipfs/repo".CurrentCommit=7a98bcb52" ./cmd/ipfs, is it right?

CurrentCommit is under the package ipfs/go-ipfs, therefore, I think it should be modify to

go install -ldflags="-X "github.com/ipfs/go-ipfs".CurrentCommit=7a98bcb52" ./cmd/ipfs

@overbool
feat(daemon): print version
7d7bca0 
License: MIT
Signed-off-by: Overbool <overbool.xu@gmail.com>
@kevina
Copy link
Contributor

kevina commented Sep 21, 2018 •
edited
Loading

Not sure. @magik6k git blame shows you last modifying the relevant line in cmd/ipfs/Rules.mk

b65cf84a (Łukasz Magiera  2018-07-23 19:11:39 +0200 16) $(d)_flags =-ldflags="-X "github.com/ipfs/go-ipfs/repo".CurrentCommit=$(git-hash)"

Commit b65cf84

@magik6k magik6k mentioned this pull request Sep 21, 2018
Merged
@magik6k
Copy link
Member

magik6k commented Sep 21, 2018

No, this isn't right, pushed a fix in #5507

kevina
kevina approved these changes Sep 21, 2018
View reviewed changes
Copy link
Contributor

@kevina kevina left a comment

Choose a reason for hiding this comment

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

LGTM.

We should modify printVersion() to take an io.Writer and fix the ipfs version command to use printVersion but that can be done in a separate commit.

@Stebalien Stebalien merged commit 238bd01 into ipfs:master Sep 21, 2018
@Stebalien
Copy link
Member

(:+1: to what @kevina said)

@overbool overbool deleted the feat/print-version branch September 22, 2018 00:18
@overbool
Copy link
Contributor Author

@Stebalien @kevina Which package should we use to put some common functions(like printVersion, maybe will be used by cmd/ipfs/daemon.go and core/commands)

@kevina
Copy link
Contributor

kevina commented Sep 22, 2018

@overbool core/commands, likely in the file version.go. cmd/ipfs/daemon.go already imports core/commands, so to do it the other way will create a circular dependency.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kevina kevina kevina approved these changes

@Stebalien Stebalien Stebalien approved these changes

@Kubuxu Kubuxu Awaiting requested review from Kubuxu

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@overbool @kevina @magik6k @Stebalien