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.

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

Print commit SHA256 when invoking "make install" #3786

Merged
merged 1 commit into from
Aug 16, 2019

Conversation

martinmr
Copy link
Contributor

@martinmr martinmr commented Aug 10, 2019

This change is Reviewable

@martinmr martinmr requested review from manishrjain and a team as code owners August 10, 2019 00:13
Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

✅ A review job has been created and sent to the PullRequest network.


@martinmr you can click here to see the review status or cancel the code review job.

Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

1 Message
The description of this pull request is blank. Adding a high-level summary will help our reviewers provide better feedback.

Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

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

Is this based on some request? I had a chat with Daniel about Dgraph binary printing out the SHA256 checksum of the binary itself.

Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @danielmai and @manishrjain)

Copy link
Contributor Author

@martinmr martinmr left a comment

Choose a reason for hiding this comment

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

I was looking at unassigned tasks in asana and this was there.

https://app.asana.com/0/1134394294595169/1134548811140584/f

Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @danielmai)

Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

This is a correct fix for echoing the sha256 git hash


Reviewed with ❤️ by PullRequest

@danielmai
Copy link
Contributor

danielmai commented Aug 13, 2019

Is this based on some request? I had a chat with Daniel about Dgraph binary printing out the SHA256 checksum of the binary itself.

It'd be a bit weird (if not practically impossible) to embed the SHA256 sum into the Dgraph binary for dgraph version. You'd have to know upfront what the checksum is in order to insert that into the binary, and the sha256sum afterward would need to be same. But it shouldn't be the same since changing the version info in dgraph version makes the checksum different. It's a chicken-and-the-egg problem.

Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

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

Yeah, I thought about the flaw in that requested logic later. But, we technically don't need to embed the SHA-256 in the binary itself. Could the binary "locate" itself and do a SHA-256 checksum at runtime?

If it is hard, don't worry about it.

Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @danielmai)

Copy link
Contributor Author

@martinmr martinmr left a comment

Choose a reason for hiding this comment

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

I can look into that in a separate PR. I still think it's useful to output the commit checksum when running make install

Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @danielmai)

Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

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

:lgtm: sure.

Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @danielmai)

@martinmr martinmr merged commit c2f2b0e into master Aug 16, 2019
@martinmr martinmr deleted the martinmr/make-commit-sha branch August 16, 2019 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants