fix(make): run UI build container as current user to avoid yarn build fail after make. #3635

shovanmaity · 2019-06-12T19:41:13Z

We are building ui-dist in make using docker image of nodejs after building dist it put those dist in build or build-external folder. These folders are created by docker run under root user. After make if we try to build it locally by yarn build it tries to delete build folder from $USER user thats why we are getting permission error.

shovan@probot:~/Desktop/workspace/src/github.com/weaveworks/scope/client$ ls -l
total 420
drwxr-xr-x    7 shovan shovan   4096 Jun 12 15:46 app
drwxr-xr-x    2 root   root     4096 Jun 13 00:42 build
drwxr-xr-x    2 root   root     4096 Jun 13 00:37 build-external
-rw-r--r--    1 shovan shovan    608 Jun 12 15:46 Dockerfile
drwxr-xr-x 1016 shovan shovan  36864 Jun 13 00:36 node_modules
-rw-r--r--    1 shovan shovan   4642 Jun 13 00:29 package.json
-rw-r--r--    1 shovan shovan   1764 Jun 12 15:46 README.md
-rw-r--r--    1 shovan shovan   2623 Jun 12 15:46 server.js
drwxr-xr-x    5 shovan shovan   4096 Jun 12 16:14 test
-rw-r--r--    1 shovan shovan   4239 Jun 12 15:46 webpack.local.config.js
-rw-r--r--    1 shovan shovan   3856 Jun 13 00:27 webpack.production.config.js
-rw-r--r--    1 shovan shovan 334991 Jun 12 15:46 yarn.lock

Fixes #3612

Signed-off-by: Shovan Maity shovan.cse91@gmail.com

satyamz · 2019-06-13T05:47:25Z

Hi @imazik Thanks for the PR. I have triggered build for this.

satyamz · 2019-06-13T06:45:34Z

Integration tests are still failing.
Hi @bboreham do you any idea on this? Result of the tests:

#!/bin/bash -eo pipefail
goveralls -repotoken $COVERALLS_REPO_TOKEN -coverprofile=profile.cov -service=circleci
exit status 2: warning: no packages being tested depend on github.com/weaveworks/scope/app/multitenant
warning: no packages being tested depend on github.com/weaveworks/scope/cri/runtime
warning: no packages being tested depend on github.com/weaveworks/scope/extras/copyreport
warning: no packages being tested depend on github.com/weaveworks/scope/extras/dialer/src
warning: no packages being tested depend on github.com/weaveworks/scope/extras/example/searchapp
warning: no packages being tested depend on github.com/weaveworks/scope/extras/example/shout
warning: no packages being tested depend on github.com/weaveworks/scope/extras/fixprobe
warning: no packages being tested depend on github.com/weaveworks/scope/probe/appclient
warning: no packages being tested depend on github.com/weaveworks/scope/probe/cri
warning: no packages being tested depend on github.com/weaveworks/scope/probe/plugins
warning: no packages being tested depend on github.com/weaveworks/scope/prog
warning: no packages being tested depend on github.com/weaveworks/scope/prog/externalui
warning: no packages being tested depend on github.com/weaveworks/scope/prog/staticui
warning: no packages being tested depend on github.com/weaveworks/scope/render/expected
warning: no packages being tested depend on github.com/weaveworks/scope/test
warning: no packages being tested depend on github.com/weaveworks/scope/test/fixture
warning: no packages being tested depend on github.com/weaveworks/scope/test/utils
warning: no packages being tested depend on github.com/weaveworks/scope/test/weave
warning: no packages being tested depend on github.com/weaveworks/scope/tools/cover
warning: no packages being tested depend on github.com/weaveworks/scope/tools/runner
warning: no packages being tested depend on github.com/weaveworks/scope/tools/socks
# github.com/weaveworks/scope/probe/docker
/tmp/go-build105652850/github.com/weaveworks/scope/probe/docker/_obj/container.go:328:17: undefined: namespaceIPAddresses
# github.com/weaveworks/scope/vendor/sigs.k8s.io/yaml
vendor/sigs.k8s.io/yaml/yaml.go:42:48: undefined: DisallowUnknownFields
# github.com/weaveworks/scope/vendor/k8s.io/client-go/transport
vendor/k8s.io/client-go/transport/round_trippers.go:437:9: undefined: strings.Builder
# github.com/weaveworks/scope/vendor/github.com/weaveworks/tcptracer-bpf/vendor/github.com/iovisor/gobpf/elf
In file included from vendor/github.com/weaveworks/tcptracer-bpf/vendor/github.com/iovisor/gobpf/elf/elf.go:52:0:
./include/bpf.h:12:30: fatal error: linux/bpf_common.h: No such file or directory
 #include <linux/bpf_common.h>
                              ^
compilation terminated.
# github.com/weaveworks/scope/vendor/github.com/google/gopacket/pcap
vendor/github.com/google/gopacket/pcap/pcap.go:22:18: fatal error: pcap.h: No such file or directory
 #include <pcap.h>
                  ^
compilation terminated.
FAIL	github.com/weaveworks/scope/app [build failed]

Exited with code 1

bboreham · 2019-06-13T14:04:04Z

The integration tests are not supposed to run for external pull requests - they would gain access to credentials which could then be used for botnets, etc. The fact that something does run which then fails in an odd way is accidental.

bboreham · 2019-06-13T14:04:31Z

Makefile

 	if test "true" != "$(SCOPE_SKIP_UI_ASSETS)"; then \
+		rm -rf client/build; \


This seems to be deleting the cache on every build?

Yes @bboreham .

@bboreham That's actually what CleanWebpackPlugin is already doing in our current setup and I imagine that's exactly the first point of failure which makes #3612 occur in the first place.

fbarl

Thanks for your PR @imazik and for capturing the issue!

I suggested we keep things simple and omit introducing new build folders if possible, let us know what you think :)

fbarl · 2019-06-13T17:19:20Z

.gitignore

@@ -59,7 +59,7 @@ extras/copyreport/copyreport
 app/static.go
 vendor/github.com/ugorji/go/codec/codecgen/bin/*
 *.codecgen.go
-client/build-external/*
+client/build*


fbarl · 2019-06-13T17:22:14Z

Makefile

-			$(SCOPE_UI_BUILD_IMAGE) yarn run build-external; \
+			$(SCOPE_UI_BUILD_IMAGE) yarn run build-external-docker; \
+		mkdir client/build-external; \
+		cp -r client/build-docker-external/* client/build-external; \


Hmm, I think having 4 different build output dirs might be a bit of an overkill, especially given the docker ones are just temporary in a sense :)

Would your commands with rm -rf above be enough to fix #3612? If yes, maybe we can just keep things as simple as possible and only keep those lines, wdyt @imazik?

Yes I can clean build directory created by root user so that we will have 2 different build dirs only. It will be simple then. 👍

fbarl · 2019-06-13T17:24:08Z

Makefile

 	if test "true" != "$(SCOPE_SKIP_UI_ASSETS)"; then \
+		rm -rf client/build; \


@bboreham That's actually what CleanWebpackPlugin is already doing in our current setup and I imagine that's exactly the first point of failure which makes #3612 occur in the first place.

fbarl · 2019-06-13T17:25:47Z

Makefile

 	if test "true" != "$(SCOPE_SKIP_UI_ASSETS)"; then \
+		rm -rf client/build-external; \


Would be nice to re-use yarn clean-build-external here if possible..

Yup I was thinking like that. Will add. Thanks.

fbarl · 2019-06-13T17:26:06Z

Makefile

-			$(SCOPE_UI_BUILD_IMAGE) yarn run build; \
+			$(SCOPE_UI_BUILD_IMAGE) yarn run build-docker; \
+		mkdir -p client/build; \
+		cp -r client/build-docker/* client/build; \


See my comment below

shovanmaity · 2019-06-14T14:51:21Z

@bboreham and @fbarl I have added cleanup steps after build PTAL.
Thank You.

bboreham · 2019-06-25T11:11:01Z

I feel this is very complicated. Is it possible to run as the current user? (mentioned at #3612 (comment))

pass user and group id in docker run command to run it as current user Signed-off-by: Shovan Maity <shovan.cse91@gmail.com>

shovanmaity · 2019-10-14T11:33:53Z

I feel this is very complicated. Is it possible to run as the current user? (mentioned at #3612 (comment))

Please check it.

fbarl

Great improvement @imazik, LGTM! 👍

qiell

Nice improvement
Thanks @imazik for fixing it.
LGTM 🎉

shovanmaity force-pushed the ui-build-fix branch from a37f8c6 to d010313 Compare June 13, 2019 04:27

satyamz requested a review from fbarl June 13, 2019 06:12

bboreham reviewed Jun 13, 2019

View reviewed changes

fbarl reviewed Jun 13, 2019

View reviewed changes

shovanmaity force-pushed the ui-build-fix branch from 652588a to 258305d Compare June 14, 2019 14:35

fbarl mentioned this pull request Oct 7, 2019

Support running / triggering tests for external contributors' PRs weaveworks/wksctl#93

Open

shovanmaity force-pushed the ui-build-fix branch from 258305d to 86cb318 Compare October 14, 2019 11:26

shovanmaity requested review from qiell and satyamz as code owners October 14, 2019 11:26

update build to run as current user

95b2317

pass user and group id in docker run command to run it as current user Signed-off-by: Shovan Maity <shovan.cse91@gmail.com>

shovanmaity force-pushed the ui-build-fix branch from 86cb318 to 95b2317 Compare October 14, 2019 11:32

fbarl approved these changes Oct 14, 2019

View reviewed changes

qiell approved these changes Oct 14, 2019

View reviewed changes

fbarl merged commit 83f9d4e into weaveworks:master Oct 14, 2019

bboreham changed the title ~~[fix] [make] update ui build to avoid yarn build fail after make.~~ fix(make): run UI build container as current user to avoid yarn build fail after make. Oct 22, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(make): run UI build container as current user to avoid yarn build fail after make. #3635

fix(make): run UI build container as current user to avoid yarn build fail after make. #3635

shovanmaity commented Jun 12, 2019 •

edited by fbarl

Loading

satyamz commented Jun 13, 2019

satyamz commented Jun 13, 2019

bboreham commented Jun 13, 2019

bboreham Jun 13, 2019

shovanmaity Jun 13, 2019

fbarl Jun 13, 2019

fbarl left a comment

fbarl Jun 13, 2019

fbarl Jun 13, 2019

shovanmaity Jun 13, 2019

fbarl Jun 13, 2019

fbarl Jun 13, 2019

shovanmaity Jun 13, 2019

fbarl Jun 13, 2019

shovanmaity commented Jun 14, 2019

bboreham commented Jun 25, 2019

shovanmaity commented Oct 14, 2019

fbarl left a comment

qiell left a comment

		if test "true" != "$(SCOPE_SKIP_UI_ASSETS)"; then \
		rm -rf client/build; \

		if test "true" != "$(SCOPE_SKIP_UI_ASSETS)"; then \
		rm -rf client/build-external; \

fix(make): run UI build container as current user to avoid yarn build fail after make. #3635

fix(make): run UI build container as current user to avoid yarn build fail after make. #3635

Conversation

shovanmaity commented Jun 12, 2019 • edited by fbarl Loading

satyamz commented Jun 13, 2019

satyamz commented Jun 13, 2019

bboreham commented Jun 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fbarl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shovanmaity commented Jun 14, 2019

bboreham commented Jun 25, 2019

shovanmaity commented Oct 14, 2019

fbarl left a comment

Choose a reason for hiding this comment

qiell left a comment

Choose a reason for hiding this comment

shovanmaity commented Jun 12, 2019 •

edited by fbarl

Loading