-
Notifications
You must be signed in to change notification settings - Fork 61
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
Cloud mta build tool docker images #997
Conversation
new file: Dockerfile_mbtci_java17 new file: Dockerfile_mbtci_java8 new file: docker-compose.test.yml new file: scripts/build_image new file: scripts/publish_image new file: test/goss/README.MD new file: test/goss/goss_template.yaml modified: .circleci/config.yml modified: cmd/testdata/mta/node-js/package.json modified: go.mod modified: go.sum modified: integration/testdata/mta_assemble/node/package.json modified: integration/testdata/mta_demo/node-js/package.json modified: integration/testdata/mta_demo/node/package.json modified: internal/artifacts/testdata/mta/node-js/package.json modified: internal/exec/testdata/mta/node-js/package.json
modified: cmd/testdata/mta/node-js/package.json modified: go.mod modified: go.sum modified: integration/testdata/mta_assemble/node/package.json modified: integration/testdata/mta_demo/node-js/package.json modified: integration/testdata/mta_demo/node/package.json modified: internal/artifacts/testdata/mta/node-js/package.json modified: internal/exec/testdata/mta/node-js/package.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend another person from your group will review this as a KT opportunity,
…cess the rest step will run
apt-get install --yes --no-install-recommends \ | ||
build-essential \ | ||
python2.7 \ | ||
python3 && \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation issue.
wget \ | ||
unzip && \ | ||
|
||
rm -rf /var/lib/apt/lists/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation issue making the code unreadable.
# so this argument's value may need to be updated in the future | ||
# Also see JVM section here: | ||
# - https://tools.hana.ondemand.com/#cloud | ||
ARG JAVA_VERSION=8.1.086 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The whole purpose of using ARG in Dockerfile is to permit to use the same one to build images with different version: see https://github.tools.sap/E-Mobility/mbt-docker/pull/8 as an implementation example.
|
||
# install java | ||
mkdir -p /opt && \ | ||
wget --no-check-certificate --no-cookies --header "Cookie: eula_3_1_agreed=tools.hana.ondemand.com/developer-license-3_1.txt; path=/;" -S https://tools.hana.ondemand.com/additional/sapjvm-${JAVA_VERSION}-linux-x64.zip && \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Security issue: it's not acceptable to not check the certificate chain of a binary distribution endpoint.
|
||
ENV PATH=$PATH:./node_modules/.bin HOME=${MTA_USER_HOME} | ||
WORKDIR /project | ||
USER mta |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All binaries are installed without doing at least an integrity check of the archive: you are then distributing build env that can't guarantee the integrity of the tools inside (needless to say their authenticity). It's a major security issue in the images.
See https://github.tools.sap/E-Mobility/mbt-docker/blob/main/Dockerfile for a proper way to do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very nice that you have decided to return Docker images back! 👍 |
Before submitting a pull request, please ensure the following:
Description
The feature is publish MBT to Dockerhub, npmjs and Github container.
For java version, it support java8, java11 and java 17
For node version, it support node14, node16 and node 18
For python version, it support python3 and python2.7
Notice, for this commit, we only generate java 17 & node 16 version, which to satisfy the new requirement from BAS team.
But the code include all java and node version combination, after we run the publish process without a problem, we will support rest java and node version combinations.
Doc info:
(1) Plan to support : https://wiki.one.int.sap/wiki/pages/viewpage.action?pageId=3138611365
(2) How to generate MTA Build Tool docker image: https://wiki.one.int.sap/wiki/display/LCNCBJ/How+to+generate+MTA+Build+Tool+docker+image
(3) Cloud MBT Release Process
Checklist