Use build template by thockin · Pull Request #20 · kubernetes/git-sync

thockin · 2016-10-28T15:56:24Z

Faster builds and versions from tags. Also use alpine as a base image. We need alpine for other architectures...

Fixes #10

This change is

thockin · 2016-10-28T15:56:51Z

This builds on #19, so we should merge that first

thockin · 2016-10-28T16:02:33Z

I forgot about the issue in #10 - will have to think about it.

DO NOT MERGE

thockin · 2016-10-28T16:12:16Z

I think I can just install coreutils. Building now.

stp-ip · 2016-10-28T16:13:56Z

For me tests fail as the git-sync location isn't changed within the test_e2e.sh.
Should move from /bin/git-sync-amd64 to /bin/amd64/git-sync

thockin · 2016-10-28T16:15:35Z

Fixed. I'll also update the test to run the docker image.

On Fri, Oct 28, 2016 at 9:14 AM, Michael Grosser notifications@github.com
wrote:

For me tests fail as the git-sync location isn't changed within the
test_e2e.sh.
Should move from /bin/git-sync-amd64 to /bin/amd/git-sync

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#20 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AFVgVNb9z-uxys6t3gP4N_qBi6_BCqrWks5q4h9IgaJpZM4Kjlwm
.

stp-ip · 2016-10-28T16:19:02Z

Makefile

+# Set default base image dynamically for each arch
 ifeq ($(ARCH),amd64)
-	docker tag $(IMAGE):$(TAG) $(LEGACY_AMD64_IMAGE):$(TAG)
+    BASEIMAGE?=alpine


Should we really not version alpine here? Probably better to be explicit with

BASEIMAGE?=alpine:3.4

stp-ip · 2016-10-28T16:28:28Z

Yeah coreutils would fix #10, but add ~6mb. Probably worth it considering shaving off almost 100 due to moving to alpine.
Awesome work.

thockin · 2016-10-28T16:33:56Z

Test runs the docker image now. PTAL

stp-ip · 2016-10-28T16:44:28Z

Makefile

+	    -v $$(pwd)/.go:/go                                                 \
+	    -v $$(pwd):/go/src/$(PKG)                                          \
+	    -v $$(pwd)/bin/$(ARCH):/go/bin                                     \
+	    -v $$(pwd)/bin/$(ARCH):/go/bin/linux_$(ARCH)                       \


This line seems unnecessary.

thockin · 2016-10-28T16:57:50Z

It is needed when building other architectures. I have a template Makefile
that this derives from, which is used in a bunch of other kube repos and
subdirs :)

On Fri, Oct 28, 2016 at 9:44 AM, Michael Grosser notifications@github.com
wrote:

@stp-ip requested changes on this pull request.

In Makefile
#20 (review)
:
+all-container: $(addprefix container-, $(ALL_ARCH))
+
+all-push: $(addprefix push-, $(ALL_ARCH))
+
+build: bin/$(ARCH)/$(BIN)
+
+bin/$(ARCH)/$(BIN): build-dirs
@echo "building: $@"

@docker run \
-ti                                                                \
-u $$(id -u):$$(id -g)                                             \
-v $$(pwd)/.go:/go                                                 \
-v $$(pwd):/go/src/$(PKG)                                          \
-v $$(pwd)/bin/$(ARCH):/go/bin                                     \
-v $$(pwd)/bin/$(ARCH):/go/bin/linux_$(ARCH)                       \
This line seems unnecessary.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#20 (review),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AFVgVIk99Rmpq88j_EoO4oHTsjOUJGQ-ks5q4iZ1gaJpZM4Kjlwm
.

stp-ip · 2016-10-28T17:07:24Z

Makefile

+	@docker build -t $(IMAGE):$(VERSION) -f .dockerfile-$(ARCH) .
+	@docker images -q $(IMAGE):$(VERSION) > $@
+	@if [ "$(ARCH)" == "amd64" ]; then \
+	    docker tag -f $(IMAGE):$(VERSION) $(LEGACY_IMAGE):$(VERSION); \


-f is deprecated and actually removed in docker 1.12. Just remove it for builds with docker 1.10+

thockin · 2016-10-28T17:10:14Z

my desktop is still 1.9 - I'll have to upgrade :)

On Fri, Oct 28, 2016 at 10:07 AM, Michael Grosser notifications@github.com
wrote:

@stp-ip commented on this pull request.

In Makefile
#20 (review)
:
      ./build/build.sh                                               \
"
+DOTFILE_IMAGE = $(subst /,_,$(IMAGE))-$(VERSION)
+
+container: .container-$(DOTFILE_IMAGE) container-name
+.container-$(DOTFILE_IMAGE): bin/$(ARCH)/$(BIN) Dockerfile.in
@Sed \
-e 's|ARG_BIN|$(BIN)|g' \
-e 's|ARG_ARCH|$(ARCH)|g' \
-e 's|ARG_FROM|$(BASEIMAGE)|g' \
Dockerfile.in > .dockerfile-$(ARCH)
@docker build -t $(IMAGE):$(VERSION) -f .dockerfile-$(ARCH) .

@docker images -q $(IMAGE):$(VERSION) > $@

@if [ "$(ARCH)" == "amd64" ]; then \
docker tag -f $(IMAGE):$(VERSION) $(LEGACY_IMAGE):$(VERSION); \
-f is deprecated and actually removed in docker 1.12. Just remove it for
builds with docker 1.10+

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#20 (review),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AFVgVKDjBTf38v1D0wDJsdAsIRlNobycks5q4ivNgaJpZM4Kjlwm
.

thockin · 2016-10-28T17:12:19Z

hmm, no upgrade available from corp...

On Fri, Oct 28, 2016 at 10:09 AM, Tim Hockin thockin@google.com wrote:

my desktop is still 1.9 - I'll have to upgrade :)

On Fri, Oct 28, 2016 at 10:07 AM, Michael Grosser <
notifications@github.com> wrote:
@stp-ip commented on this pull request.

In Makefile
#20 (review)
:
     ./build/build.sh                                               \
   "
+DOTFILE_IMAGE = $(subst /,_,$(IMAGE))-$(VERSION)
+
+container: .container-$(DOTFILE_IMAGE) container-name
+.container-$(DOTFILE_IMAGE): bin/$(ARCH)/$(BIN) Dockerfile.in
@Sed \
   -e 's|ARG_BIN|$(BIN)|g' \
   -e 's|ARG_ARCH|$(ARCH)|g' \
   -e 's|ARG_FROM|$(BASEIMAGE)|g' \
   Dockerfile.in > .dockerfile-$(ARCH)
@docker build -t $(IMAGE):$(VERSION) -f .dockerfile-$(ARCH) .

@docker images -q $(IMAGE):$(VERSION) > $@

@if [ "$(ARCH)" == "amd64" ]; then \
   docker tag -f $(IMAGE):$(VERSION) $(LEGACY_IMAGE):$(VERSION); \
-f is deprecated and actually removed in docker 1.12. Just remove it for
builds with docker 1.10+

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#20 (review),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AFVgVKDjBTf38v1D0wDJsdAsIRlNobycks5q4ivNgaJpZM4Kjlwm
.

stp-ip · 2016-10-28T17:35:21Z

test_e2e.sh

 echo "Building..."
-make >/dev/null
-GIT_SYNC=./bin/git-sync-amd64
+make container REGISTRY=e2e TAG=$(make version) >/dev/null


For me the $(make version) is not silenced and therefore is assigned "Entering".
Would use $(make -s version) instead.

stp-ip · 2016-10-28T17:35:33Z

test_e2e.sh

+        -i \
+        -u $(id -u):$(id -g) \
+        -v "$DIR":"$DIR" \
+        e2e/git-sync-amd64:$(make version) \


Same here. Would use $(make -s version) instead.

Faster builds and versions from tags. Also use alpine as a base image. We need alpine for other architectures... Also change the --wait flag to take a float for sub-second waits.

thockin · 2016-10-31T00:09:15Z

Fixed in new push, passes e2e.

TaiSHiNet · 2016-10-31T11:29:22Z

Awesome!! Please let me know when you push this to grc, I'll grab the tag (tried v2.0.3 already and nope)
Thanks for your work!

stp-ip · 2016-10-31T11:37:14Z

We'll have to wait for thockin on the push. But I assume this will happen soon. Will let you know.

thockin · 2016-11-02T06:23:06Z

Pushed to GCR

On Mon, Oct 31, 2016 at 12:37 PM, Michael Grosser notifications@github.com
wrote:

We'll have to wait for thockin on the push. But I assume this will happen
soon. Will let you know.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#20 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AFVgVAIcPgMZX8Ihv_EYAE3JBF3gUmEoks5q5dLrgaJpZM4Kjlwm
.

thockin assigned bowei Oct 28, 2016

thockin force-pushed the use-build-template branch from cd1026d to 836e10b Compare October 28, 2016 16:15

stp-ip requested changes Oct 28, 2016

View reviewed changes

thockin force-pushed the use-build-template branch 2 times, most recently from a501a68 to 35c89d4 Compare October 28, 2016 16:33

stp-ip requested changes Oct 28, 2016

View reviewed changes

stp-ip approved these changes Oct 28, 2016

View reviewed changes

stp-ip reviewed Oct 28, 2016

View reviewed changes

Use the go-build-template

70feeb5

Faster builds and versions from tags. Also use alpine as a base image. We need alpine for other architectures... Also change the --wait flag to take a float for sub-second waits.

thockin force-pushed the use-build-template branch from 35c89d4 to 70feeb5 Compare October 31, 2016 00:09

stp-ip merged commit 7fdc748 into kubernetes:master Oct 31, 2016

thockin deleted the use-build-template branch October 31, 2020 06:09

Conversation

thockin commented Oct 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thockin commented Oct 28, 2016

Uh oh!

thockin commented Oct 28, 2016

Uh oh!

thockin commented Oct 28, 2016

Uh oh!

stp-ip commented Oct 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thockin commented Oct 28, 2016

Uh oh!

stp-ip Oct 28, 2016

Choose a reason for hiding this comment

Uh oh!

thockin Oct 28, 2016

Choose a reason for hiding this comment

Uh oh!

stp-ip commented Oct 28, 2016

Uh oh!

thockin commented Oct 28, 2016

Uh oh!

stp-ip Oct 28, 2016

Choose a reason for hiding this comment

Uh oh!

thockin commented Oct 28, 2016

@stp-ip requested changes on this pull request.

Uh oh!

stp-ip Oct 28, 2016

Choose a reason for hiding this comment

Uh oh!

thockin commented Oct 28, 2016

@stp-ip commented on this pull request.

Uh oh!

thockin commented Oct 28, 2016

@stp-ip commented on this pull request.

Uh oh!

stp-ip Oct 28, 2016

Choose a reason for hiding this comment

Uh oh!

stp-ip Oct 28, 2016

Choose a reason for hiding this comment

Uh oh!

thockin commented Oct 31, 2016

Uh oh!

TaiSHiNet commented Oct 31, 2016

Uh oh!

stp-ip commented Oct 31, 2016

Uh oh!

thockin commented Nov 2, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

thockin commented Oct 28, 2016 •

edited

Loading

stp-ip commented Oct 28, 2016 •

edited

Loading