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

Build in Dockerfile that works for java and node #23

Merged
merged 1 commit into from
May 27, 2020
Merged

Build in Dockerfile that works for java and node #23

merged 1 commit into from
May 27, 2020

Conversation

harryherbig
Copy link
Contributor

@harryherbig harryherbig commented May 25, 2020 •
edited
Loading

  • Add Dockerfile that works for node and java
  • Use docker image from Dockerfile for make
  • fix paths in Makefile for always running in docker

TODO:

  • publish docker image on hub.docker.com e.g. to use it
  • clean package.json from obsolete dependencies

@github-actions
Copy link

✅ No Breaking Changes

harryherbig
harryherbig commented May 25, 2020
View reviewed changes
Makefile Outdated Show resolved Hide resolved
harryherbig
harryherbig commented May 25, 2020
View reviewed changes
Makefile Outdated
ifeq ($(LANGUAGE),node)
FLAGS+= --plugin=protoc-gen-ts=$(NODE_DIR)/node_modules/.bin/protoc-gen-ts
FLAGS+= --plugin=protoc-gen-grpc=$(NODE_DIR)/node_modules/.bin/grpc_tools_node_protoc_plugin
FLAGS+= --plugin=protoc-gen-ts=/node_modules/.bin/protoc-gen-ts
Copy link
Contributor Author

Choose a reason for hiding this comment

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

paths are absolut cause we know the location in the image

Copy link
Member

@moritzzimmer moritzzimmer May 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

FLAGS+= --grpc_out=$(OUTPUT) should probably be FLAGS+= --grpc_out=$(NODE_DIR) so we don't create node stuff in the java output - this probably fixes the clean issue as well

harryherbig
harryherbig commented May 25, 2020
View reviewed changes
Makefile Show resolved Hide resolved
harryherbig
harryherbig commented May 25, 2020
View reviewed changes
Makefile
@@ -51,8 +51,8 @@ clean: ## Deletes all generated files
@echo "+ $@"
rm -rf $(JAVA_DIR) || true
rm -rf $(GO_DIR) || true
rm -R `find node -type d \( -iname "*" ! -iname "node_modules" ! -iname "tests" \) -mindepth 1 -maxdepth 1` || true
rm -rf `find $(NODE_DIR) -type d \( -iname "*" ! -iname "node_modules" ! -iname "tests" \) -mindepth 1 -maxdepth 1` 2> /dev/null || true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

get rid of usage printout of rm when no files need to be deleted by passing the errout to /dev/null

@github-actions
Copy link

✅ No Breaking Changes

2 similar comments
@github-actions
Copy link

✅ No Breaking Changes

@github-actions
Copy link

✅ No Breaking Changes

bobaaaaa
bobaaaaa reviewed May 25, 2020
View reviewed changes
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
@github-actions
Copy link

✅ No Breaking Changes

4 similar comments
@github-actions
Copy link

✅ No Breaking Changes

@github-actions
Copy link

✅ No Breaking Changes

@github-actions
Copy link

✅ No Breaking Changes

@github-actions
Copy link

✅ No Breaking Changes

moritzzimmer
moritzzimmer reviewed May 25, 2020
View reviewed changes
Dockerfile Outdated Show resolved Hide resolved
moritzzimmer
moritzzimmer reviewed May 25, 2020
View reviewed changes
.github/workflows/pull-request.yml Outdated
@@ -52,17 +52,14 @@ jobs:
steps:
- uses: actions/checkout@v2

- name: make
run: make LANGUAGE=java
Copy link
Member

Choose a reason for hiding this comment

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

still the default 👍 (so could be just make)

moritzzimmer
moritzzimmer reviewed May 25, 2020
View reviewed changes
Copy link
Member

@moritzzimmer moritzzimmer left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Nice, does the job 👍 I just think the resulting image could be optimized in terms of size and the Makefile does contain some now deprecated comments and a glitch in the node config

@github-actions
Copy link

✅ No Breaking Changes

10 similar comments
@github-actions
Copy link

✅ No Breaking Changes

@github-actions
Copy link

✅ No Breaking Changes

@github-actions
Copy link

✅ No Breaking Changes

@github-actions
Copy link

✅ No Breaking Changes

@github-actions
Copy link

✅ No Breaking Changes

@github-actions
Copy link

✅ No Breaking Changes

@github-actions
Copy link

✅ No Breaking Changes

@github-actions
Copy link

✅ No Breaking Changes

@github-actions
Copy link

✅ No Breaking Changes

@github-actions
Copy link

✅ No Breaking Changes

@github-actions
Copy link

✅ No Breaking Changes

1 similar comment
@github-actions
Copy link

✅ No Breaking Changes

@moritzzimmer moritzzimmer marked this pull request as ready for review May 27, 2020 07:50
@moritzzimmer moritzzimmer added the 💅 enhancement New feature or request label May 27, 2020
@github-actions
Copy link

✅ No Breaking Changes

moritzzimmer
moritzzimmer approved these changes May 27, 2020
View reviewed changes
Copy link
Member

@moritzzimmer moritzzimmer left a comment

Choose a reason for hiding this comment

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

cleanup history and go!

bobaaaaa
bobaaaaa reviewed May 27, 2020
View reviewed changes
Dockerfile Outdated
RUN mkdir -p /installer/protoc
RUN unzip -o protoc.zip -d /installer/protoc/

FROM node:lts-stretch-slim
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
FROM node:lts-stretch-slim
FROM node:lts-alpine

Das dürfte noch kleiner sein.

Dockerfile Outdated
Comment on lines 23 to 26
USER root
COPY --from=installer /node_modules /node_modules
COPY --from=installer /installer/protoc /usr/bin/protoc
COPY --from=installer /installer/protoc-gen-grpc-java /usr/bin/
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
USER root
COPY --from=installer /node_modules /node_modules
COPY --from=installer /installer/protoc /usr/bin/protoc
COPY --from=installer /installer/protoc-gen-grpc-java /usr/bin/
USER node
COPY --chown=node:node --from=installer /node_modules /node_modules
COPY --chown=node:node --from=installer /installer/protoc /usr/bin/protoc
COPY --chown=node:node --from=installer /installer/protoc-gen-grpc-java /usr/bin/

Als root ist nicht so gut, oder?

@github-actions
Copy link

✅ No Breaking Changes

1 similar comment
@github-actions
Copy link

✅ No Breaking Changes

@harryherbig
Copy link
Contributor Author

merging with root user because with node user we got permission denied during CI.
merging without alpine because of permission problems during docker build

@github-actions
Copy link

✅ No Breaking Changes

@harryherbig harryherbig merged commit 1c0dc51 into master May 27, 2020
@harryherbig harryherbig deleted the make-in-docker branch May 27, 2020 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bobaaaaa bobaaaaa bobaaaaa left review comments

@moritzzimmer moritzzimmer moritzzimmer approved these changes

Assignees

@harryherbig harryherbig

Labels
💅 enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@harryherbig @bobaaaaa @moritzzimmer