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.

Sign up for GitHub

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

Jump to bottom

docker: fix ipfs version --commit #2734

Merged
merged 2 commits into from
May 20, 2016
Merged

docker: fix ipfs version --commit #2734

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6b94fde
docker: fix CurrentCommit discovery
May 18, 2016
7145d30
docker: add test for ipfs version --commit
May 18, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,8 @@ RUN apk add --update musl go=$GO_VERSION git bash wget ca-certificates \
# We get the current commit using this hack,
# so that we don't have to copy all of .git/ into the build context.
# This saves us quite a bit of image size.
&& ref="$(cat .git/HEAD | cut -d' ' -f2)" \
&& commit="$(cat .git/$ref | head -c 7)" \
&& ref=$(cat .git/HEAD | grep ref | cut -d' ' -f2) \
&& commit=$(if [ -z "$ref" ]; then cat .git/HEAD; else cat ".git/$ref"; fi | head -c 7) \
Copy link
Contributor

Choose a reason for hiding this comment

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

To get the current commit it's better to use git rev-parse --short HEAD and to get the current branch git symbolic-ref --short HEAD 2>/dev/null || echo "$commit" (where $commit is the current commit).

Copy link
Author

Choose a reason for hiding this comment

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

Hey thanks for wighing in! -- do you happen to know if rev-parse and symbolic-ref work without an actual repo, i.e. with only .git/HEAD and .git/refs or something similar present? The idea with this hack is that we don't have to copy all of .git, and thus gain savings in image size.

Copy link
Contributor

Choose a reason for hiding this comment

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

You need the following for rev-parse and symbolic-ref to work:

  • .git/HEAD
  • .git/refs/
  • .git/objects/ which can be empty

See: https://github.com/git/git/blob/master/setup.c#L266

So it might be a good idea to just mkdir .git/objects/ so that you can use git commands afterwards.

It could be also a good idea to copy .git/packed-refs if it exists in case the refs are packed.

Copy link
Member

@jbenet jbenet Aug 26, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

i think this is working right? did we want the improvements @chriscool proposed?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, nevermind, already improved: 0eed330 👍

&& echo "ldflags=-X github.com/ipfs/go-ipfs/repo/config.CurrentCommit=$commit" \
# Build and install IPFS and entrypoint script
&& cd $SRC_PATH/cmd/ipfs \
Expand Down
4 changes: 2 additions & 2 deletions test/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,8 @@ RUN apk add --update musl go=$GO_VERSION git bash wget ca-certificates \
COPY . $SRC_PATH

RUN cd $SRC_PATH \
&& ref="$(cat .git/HEAD | cut -d' ' -f2)" \
&& commit="$(cat .git/$ref | head -c 7)" \
&& ref=$(cat .git/HEAD | grep ref | cut -d' ' -f2) \
&& commit=$(if [ -z "$ref" ]; then cat .git/HEAD; else cat ".git/$ref"; fi | head -c 7) \
&& echo "ldflags=-X github.com/ipfs/go-ipfs/repo/config.CurrentCommit=$commit" \
&& cd $SRC_PATH/cmd/ipfs \
&& go build -ldflags "-X github.com/ipfs/go-ipfs/repo/config.CurrentCommit=$commit" \
Expand Down
8 changes: 8 additions & 0 deletions test/sharness/t0300-docker-image.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,14 @@ test_expect_success "simple ipfs add/cat can be run in docker container" '
test_cmp expected actual
'

test_expect_success "version CurrentCommit is set" '
docker_exec "$DOC_ID" "wget --retry-connrefused --waitretry=1 --timeout=30 -t 30 \
-q -O - http://localhost:8080/version" | grep Commit | cut -d" " -f2 >actual &&
docker_exec "$DOC_ID" "ipfs version --commit" | cut -d- -f2 >expected &&
[ "$(cat expected | wc -c)" -gt "1" ] && # check there actually is a commit set
Copy link
Contributor

Choose a reason for hiding this comment

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

test -s expected is better than [ "$(cat expected | wc -c)" -gt "1" ].

Copy link
Author

Choose a reason for hiding this comment

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

Good suggestion

Copy link
Member

Choose a reason for hiding this comment

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

i dont think this was ever improved

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, nevermind, it was: 0eed330 👍

test_cmp expected actual
'

test_expect_success "stop docker container" '
docker_stop "$DOC_ID"
'
Expand Down