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

Use builtin go buildinfo for git version information #2186

Merged
merged 1 commit into from May 15, 2023

Conversation

mjsir911
Copy link
Contributor

This makes things more workable with offbeat build procedures eg package managers that might not fetch the repo from git, golang autoembeds this info when available & fails gracefully when not, or when manually requested with -buildinfo=false or -buildvcs=false (this is what gentoo does)

@dangra dangra requested a review from tvdfly April 26, 2023 16:42
Copy link
Member

@dangra dangra left a comment

Choose a reason for hiding this comment

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

The linter spotted some issues worth checking

@mjsir911 mjsir911 force-pushed the msirabella/idomatic_buildinfo branch from f8ac8e5 to dcc91a2 Compare April 26, 2023 18:43
@mjsir911
Copy link
Contributor Author

yeah, clued me in on the tests for this. Unfortunately I can't think of an easy way to mock debug.ReadBuildInfo() so will just remove the git revision checking for now. Going to research how it's done elsewhere though.

This makes things more workable with offbeat build procedures
eg package managers that might not fetch the repo from git,
golang autoembeds this info when available & fails gracefully when not,
or when manually requested with `-buildinfo=false` or `-buildvcs=false`
(this is what gentoo does)
@kzys
Copy link
Member

kzys commented Apr 26, 2023

I'm fine not testing that Commit part. We could have a function that takes Settings, but we really don't have much in that logic.

Copy link
Collaborator

@tvdfly tvdfly left a comment

Choose a reason for hiding this comment

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

Can we bring back the branch name for dev builds? It's helpful to see which branch something was built on when doing a bunch of local dev.

For example, this is what I have right now after running make on a branch:

% fly version
flyctl v0.0.0-1682385080+dev darwin/arm64 Commit: c62ae16a0a9fa41d3aedc56627632b7872bc4c95 (client-init-for-libs) BuildDate: 2023-04-25T01:11:20Z

we don't need (or probably want) the branch names in the builds that get released.

@mjsir911
Copy link
Contributor Author

Can we bring back the branch name for dev builds? It's helpful to see which branch something was built on when doing a bunch of local dev.

For example, this is what I have right now after running make on a branch:

% fly version
flyctl v0.0.0-1682385080+dev darwin/arm64 Commit: c62ae16a0a9fa41d3aedc56627632b7872bc4c95 (client-init-for-libs) BuildDate: 2023-04-25T01:11:20Z

we don't need (or probably want) the branch names in the builds that get released.

Hey tvdfly,

Thanks for the request!, if this is a hard requirement on the version info, this PR is pretty much a non-starter.

The git command used to embed this data is revision (and timestamp) only: -c log.showsignature=false show -s --format=%H:%ct.

The go team does seem amenable to adding more detailed data into buildinfo, but that would be a pretty drawn out process.

Related issues:

I might end up filing a proposal to add branch name (or just custom information?)

Will let y'all close out this PR if you decide to, but thanks for considering it!

@tvdfly
Copy link
Collaborator

tvdfly commented May 4, 2023

What if we make our own branch var in the place we used to set the commit sha, and then set it in the makefile? If the var is set we can show it and if not we ignore it.

@mjsir911
Copy link
Contributor Author

What if we make our own branch var in the place we used to set the commit sha, and then set it in the makefile? If the var is set we can show it and if not we ignore it.

Part of the intent of the PR was moving this tagged-info logic outside of the makefile. Doing this works but brings us back to the same initial place where building without being inside of a git repo isn't supported nicely.

Could also sunset this PR and just make something to better handle git-less builds

@tvdfly
Copy link
Collaborator

tvdfly commented May 12, 2023

@mjsir911 I made a branch on top of this one with what I'm thinking: #2284 (this commit in particular: e2a6baf6 )

The branch name just doesn't show up if git isn't installed or doesn't work or whatever.

Is that still going to be a problem in the cases you outline? It doesn't seem like it to me, and I'm not very familiar with the issues that motivated the original change. Appreciate your help and this contribution!

@tvdfly tvdfly merged commit e2fc139 into superfly:master May 15, 2023
3 checks passed
@mjsir911
Copy link
Contributor Author

@mjsir911 I made a branch on top of this one with what I'm thinking: #2284 (this commit in particular: e2a6baf6 )

The branch name just doesn't show up if git isn't installed or doesn't work or whatever.

Is that still going to be a problem in the cases you outline? It doesn't seem like it to me, and I'm not very familiar with the issues that motivated the original change. Appreciate your help and this contribution!

Sorry about the slow response, thanks for the merge! The change you linked makes everything work smoothly, yes.

I appreciate you working with me through this one.

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.

None yet

4 participants