Skip to content

Add tag & 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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ViktorTigerstrom
Copy link
Contributor

@ViktorTigerstrom ViktorTigerstrom commented Apr 14, 2025

Mimics the behaviour of loop pull 914 (lightninglabs/loop#914), but for litd.

This change adds a new field commit_hash field to the GetInfo response, which will include the litd tag if the currently running litd daemon is built on a a clean tag. If the litd version is not built on a clean tagged version, this field will instead include the commit hash that the build is based on.

The contents of version field of the GetInfo response is also will also be changed, so that the it'll only include the litd version, and not the commit_hash/tag.

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:

  1. git tag -s v123456789.123456789.123456789-alpha
  2. And build the release with make docker-release tag=v123456789.123456789.123456789-alpha if you're on mac, or make release tag=v123456789.123456789.123456789-alpha with the correct GO version if you're on Linux.
  3. Then verify that the sha256 output for the ./lightning-terminal-v123456789.123456789.123456789-alpha/manifest-v123456789.123456789.123456789-alpha.txt file is: e9bc6a43642d7ae679f1f2bd809c3a517cefc0bddb32beee5e990fc60bd75783

@ViktorTigerstrom ViktorTigerstrom changed the title 2025 04 include commit hash Add commit_hash field to GetInfo response Apr 14, 2025
@@ -54,4 +54,7 @@ message GetInfoRequest {
message GetInfoResponse {
// The version of the LiTd software that the node is running.
string version = 1;

// The git commit hash of the litd binary.
string commit_hash = 2;
Copy link
Contributor Author

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 :)

Copy link
Contributor

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.

Copy link
Contributor

@bitromortac bitromortac left a 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 🎉

@@ -54,4 +54,7 @@ message GetInfoRequest {
message GetInfoResponse {
// The version of the LiTd software that the node is running.
string version = 1;

// The git commit hash of the litd binary.
string commit_hash = 2;
Copy link
Contributor

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.

Copy link
Member

@ellemouton ellemouton left a 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?

@lightninglabs-deploy
Copy link

@ViktorTigerstrom, remember to re-request review from reviewers when ready

@ViktorTigerstrom ViktorTigerstrom force-pushed the 2025-04-include-commit-hash branch from e0b7fc5 to 90882c6 Compare May 2, 2025 07:57
Add the `tag` and `commit_hash` fields to the GetInfoResponse. 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/).

This commit also updates the `litcli --version` command result, to
include all three fields of the GetInfoResponse.
@ViktorTigerstrom ViktorTigerstrom force-pushed the 2025-04-include-commit-hash branch from 90882c6 to 49af0b0 Compare May 2, 2025 08:06
@ViktorTigerstrom
Copy link
Contributor Author

Thanks for the reviews!

I've updated the PR to address the reviews, by instead adding a tag and a commit_hash field to the GetInfo response.
The tag field will be set to the Git tag that the LiT release binary build was based on. If the build was not based on a clean tagged commit, this field will contain the most recent tag suffixed by the commit hash, and/or a "-dirty" suffix (i.e. the same behaviour as the old "-Commit=" part in the old GetInfo response).

The commit_hash field will contain the full commit hash of the commit that LiT release binary build was based on.

The contents of the version field in the GetInfo response has also been updated to only include the semantic version number of the LiT release, following the semantic versioning 2.0.0 spec (http://semver.org/).

@bitromortac & @ellemouton, can you please check the updates, and check if you think the outputs of the litcli getinfo and litcli --version commands makes sense? If you do, I'll proceed to communicate the changes to the loop team and coordinate to and ensure that we try to follow the same standard for all daemons.

@ViktorTigerstrom ViktorTigerstrom changed the title Add commit_hash field to GetInfo response Add tag & commit_hash field to GetInfo response May 2, 2025
Copy link
Contributor

@bitromortac bitromortac left a comment

Choose a reason for hiding this comment

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

LGTM, tACK 🎉

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.

4 participants