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

docker: fix ipfs version --commit #2734

Merged
merged 2 commits into from
May 20, 2016
Merged

Conversation

ghost
Copy link

@ghost ghost commented May 18, 2016

The dockerfile assumes we're always on a branch, in which case it resolves the head ref in .git/HEAD via .git/refs/. Solarnet deploys are headless though, i.e. not on a branch but on a specific commit. In this case, Git puts that commit ref directly into .git/HEAD, so that the resolution step breaks.

@ghost ghost added kind/bug A bug in existing code (including security flaws) topic/containers + vms labels May 18, 2016
@ghost ghost force-pushed the fix-dockerfile-currentcommit branch from d36bc3f to 15ece2c Compare May 18, 2016 20:24
@ghost
Copy link
Author

ghost commented May 18, 2016

Fixed the signoff

@whyrusleeping
Copy link
Member

is this related to #2453 at all?

@ghost
Copy link
Author

ghost commented May 18, 2016

Oh hrm yes very much actually. And failing tests.

Let me take a look (at both).

@ghost ghost force-pushed the fix-dockerfile-currentcommit branch from 15ece2c to 547c8dc Compare May 18, 2016 21:28
@Kubuxu
Copy link
Member

Kubuxu commented May 19, 2016

I don't know docker that much but LGTM.

@Kubuxu Kubuxu added the RFM label May 19, 2016
@ghost ghost removed the RFM label May 20, 2016
@ghost ghost force-pushed the fix-dockerfile-currentcommit branch from 547c8dc to f4f61a5 Compare May 20, 2016 00:19
@ghost
Copy link
Author

ghost commented May 20, 2016

ok now this one is fixed too

@ghost ghost added the RFM label May 20, 2016
@@ -37,6 +37,7 @@ ENV SRC_PATH /go/src/github.com/ipfs/go-ipfs
# Get the go-ipfs sourcecode
COPY . $SRC_PATH

RUN apk add --update git
Copy link
Author

Choose a reason for hiding this comment

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

Oops, pushing one more update to remove this stray line

Lars Gierth added 2 commits May 20, 2016 02:40
License: MIT
Signed-off-by: Lars Gierth <larsg@systemli.org>
License: MIT
Signed-off-by: Lars Gierth <larsg@systemli.org>
@ghost ghost force-pushed the fix-dockerfile-currentcommit branch from f4f61a5 to 7145d30 Compare May 20, 2016 00:40
@whyrusleeping whyrusleeping merged commit 675b5b2 into master May 20, 2016
@whyrusleeping whyrusleeping deleted the fix-dockerfile-currentcommit branch May 20, 2016 22:07
&& 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

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 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug A bug in existing code (including security flaws) RFM topic/containers + vms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants