-
Notifications
You must be signed in to change notification settings - Fork 109
Add commit_hash field to GetInfo response
#1034
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
Add commit_hash field to GetInfo response
#1034
Conversation
commit_hash field to GetInfo response
| string version = 1; | ||
|
|
||
| // The git commit hash of the litd binary. | ||
| string commit_hash = 2; |
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.
Let me know if you think commit would be a more suitable name for this, as the contents may be the tag rather than a commit hash. But opted for this naming as it then mimics the loop getinfo response :)
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.
Staying consistent is probably better, I would have chosen "revision" I guess.
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.
LGTM, nice ⚡
edit: will still check the build
edit2: reproduced the build 🎉
| string version = 1; | ||
|
|
||
| // The git commit hash of the litd binary. | ||
| string commit_hash = 2; |
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.
Staying consistent is probably better, I would have chosen "revision" I guess.
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.
I guess I dont love the "it's sometimes this but then sometimes that" part.
Cant we just leave "Commit" as it is (to not change behaviour there) but then add "Commit_hash" which is always the revision hash no matter what?
|
@ViktorTigerstrom, remember to re-request review from reviewers when ready |
90882c6 to
49af0b0
Compare
|
Thanks for the reviews! I've updated the PR to address the reviews, by instead adding a The The contents of the @bitromortac & @ellemouton, can you please check the updates, and check if you think the outputs of the |
commit_hash field to GetInfo responsetag & commit_hash field to GetInfo response
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.
LGTM, tACK 🎉
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.
Left some comments, let me know your thoughts :)
version.go
Outdated
|
|
||
| // GitCommitHash stores the current git commit hash of this build. This should | ||
| // be set using the -ldflags during compilation. | ||
| var GitCommitHash string |
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.
suggest just "CommitHash"
version.go
Outdated
| // CommitHash returns the git commit hash of the current build. | ||
| func CommitHash() string { | ||
| return GitCommitHash | ||
| } | ||
|
|
||
| // CommitTag returns the git tag of the current build. | ||
| func CommitTag() string { | ||
| return Commit | ||
| } |
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.
why are these functions? I'd say just use the globals directly. The reason Version is a function is cause it does other stuff too
litrpc/proxy.proto
Outdated
| // The Git commit hash the LiTd binary build was based on. | ||
| string commit_hash = 3; |
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.
perhaps the MVP here is to just add this commit_hash field and leave out the tag for now? afaiu, the initial request was just about consistently getting the commit?
Add the `commit_hash` field to the GetInfoResponse. The `commit_hash` field will contain the most recent commit_hash that the build was based on. If the build had uncommitted changes, this field will contain the most recent commit hash, suffixed by "-dirty". The semantics of the `version` field is also updated to always contain the most recent semantic version of the litd node, following the semantic versioning 2.0.0 spec (http://semver.org/).
The `commit_hash` field will contain the full commit hash of the commit that build was based on.
49af0b0 to
59ecb77
Compare
tag & commit_hash field to GetInfo responsecommit_hash field to GetInfo response
|
Updated the PR to address feedback we discussed offline: In general, this PR has been updated so that: The contents of the Additionally, a |
| make_ldflags = $(2) -X $(LND_PKG)/build.Commit=lightning-terminal-$(COMMIT) \ | ||
| -X $(LND_PKG)/build.CommitHash=$(COMMIT_HASH) \ | ||
| -X $(LND_PKG)/build.GoVersion=$(GOVERSION) \ | ||
| -X $(LND_PKG)/build.RawTags=$(shell echo $(1) | sed -e 's/ /,/g') \ |
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.
Note that we do not change the:
$(LND_PKG)/build.Commit=lightning-terminal-$(COMMIT)
above here, so that the Commit in the getinfo response through lncli remains the same. I think it doesn't make sense to change this until we also change the output of getinfo in lnd.
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.
lgtm!
Mimics the behaviour of loop pull 914 (lightninglabs/loop#914), but for
litd.This change adds a new field
commit_hashfield to theGetInforesponse, which will contain the full commit hash of the commit that LiT release binary build was based on. If the build had uncommitted changes, thecommit_hashfield will contain the most recent commit hash, suffixed by "-dirty".The contents of
versionfield of theGetInforesponse is also will also be changed, so that the it'll only include thelitdversion, and not the commit_hash/tag.A
commit_hashfield is also added to the--versionoutput, so which will always contains the full commit hash of the latest commit (with no suffix even on dirty builds).Note to reviewers:
Just to ensure that this doesn't break the reproducible build across different hosts:
Could you please create a local tag on top of this:
git tag -s v123456789.123456789.123456789-alphamake docker-release tag=v123456789.123456789.123456789-alphaif you're on mac, ormake release tag=v123456789.123456789.123456789-alphawith the correct GO version if you're on Linux.sha256output for the./lightning-terminal-v123456789.123456789.123456789-alpha/manifest-v123456789.123456789.123456789-alpha.txtfile is:e9bc6a43642d7ae679f1f2bd809c3a517cefc0bddb32beee5e990fc60bd75783