-
Notifications
You must be signed in to change notification settings - Fork 177
Conversation
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
Makefile
Outdated
ifeq ($(BUILDTIME),) | ||
BUILDTIME := $(shell date -u +"%Y-%m-%dT%H:%M:%SZ" 2> /dev/null) | ||
ifeq ($(COMMIT_TIME),) | ||
COMMIT_TIME := $(shell git show -s --format="%cd" --date=format:"%Y-%m-%dT%H:%M:%SZ" 2> /dev/null) |
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.
Do we need/want the date to be in UTC?
(I recall having to go through date
to get that, but it's been a while, and it's dirty. Perhaps there's an easier way docker/docker-ce-packaging#125)
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.
Actually this is just an input format. According to https://github.com/docker/app/blob/master/internal/version.go#L24 this will just be used as input to the function that will return in the final format in https://github.com/docker/app/blob/master/internal/version.go#L32.
Here I had a look at the FIXME
comment on line 31, but since the file does not exist in the cli, I didn't do the suggested change.
Why do we need such date? |
@ndeloof I see this more as a user feature to know from when is the commit of the current binary and not a dev/debug information. Maybe we need to confirm with @chris-crone before merging. |
Codecov Report
@@ Coverage Diff @@
## master #787 +/- ##
======================================
Coverage 70.4% 70.4%
======================================
Files 67 67
Lines 3740 3740
======================================
Hits 2633 2633
Misses 760 760
Partials 347 347 Continue to review full report at Codecov.
|
The only real use of the build time is when you're testing your local build between commits and want to make sure you're using the latest version. We can:
I'm happy with either of the above solutions. |
Was having a chat with @chris-crone (not related to this PR) Just recalled there's a "standard" env-var to use for commit-time; |
Signed-off-by: Ulysses Souza <ulyssessouza@gmail.com>
aa8ba93
to
36e84c0
Compare
I've chosen approach 1, removing build time. |
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
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.
SGTM
Windows CI is failing (not clear what on though) |
@thaJeztah I just rerun the tests in the CI. Windows machines are a bit unstable |
All green now; @chris-crone PTAL |
- What I did
Replace build time in binary with commit time
- How I did it
By changing the date by the git commiter date in the Makefile
- How to verify it
- Description for the changelog
Replace build time in binary with commit time
- A picture of a cute animal (not mandatory)