imbajin commented on code in PR #3025: URL: https://github.com/apache/hugegraph/pull/3025#discussion_r3241982190
########## .dockerignore: ########## @@ -0,0 +1,35 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# + +# NOTE: This file intentionally stays minimal. +**/target/ +# Most patterns (IDE files, build artifacts, logs, OS files) are already +# covered by .gitignore. Only Docker-specific extras are listed here. Review Comment: ‼️ The premise here is incorrect — Docker's build context exclusion does **not** inherit `.gitignore`. As a result, locally-extracted release dirs (`apache-hugegraph-*/`, measured at ~790 MB on a normal dev checkout), `.idea/`, `node_modules/`, `logs/`, `*.iml`, `*.class`, `gen-java/`, `upload-files/`, `.vscode/`, `*.tar.gz`, `dist/`, `build/` all leak into every build's context, defeating the cache work in this PR (and risking IDE / secret leakage into image layers). ```suggestion **/target/ # IMPORTANT: .dockerignore does NOT inherit .gitignore — patterns must be restated. # Pre-extracted release dirs / archives apache-hugegraph-*/ **/*.tar **/*.tar.gz* **/*.zip **/*.war # IDE / OS .idea/ .vscode/ **/*.iml **/*.iws **/.DS_Store # Build / runtime artifacts **/logs/ **/*.log **/*.class **/gen-java/ **/upload-files/ **/dist/ **/build/ **/node_modules/ # Env files .env.local .env.*.local # Git internals .git .gitignore .gitattributes .github # Compose / docs not needed in build context **/docker-compose*.yml **/docker-compose*.yaml **/*.md docs/ ``` ########## .github/workflows/server-ci.yml: ########## @@ -10,8 +10,7 @@ on: jobs: build-server: - # TODO: we need test & replace it to ubuntu-24.04 or ubuntu-latest - runs-on: ubuntu-22.04 + runs-on: ubuntu-latest Review Comment: ⚠️ Two concerns on this line: 1. **Scope creep**: bumping `ubuntu-22.04 → ubuntu-latest` is unrelated to the Docker cache work in this PR. The original `# TODO` was a deliberate guard (Ubuntu 24.04 changed default Java, removed legacy packages). Suggest reverting in this PR and splitting into a focused follow-up. 2. **`ubuntu-latest` drifts** when GitHub flips the alias — pin explicitly: ```suggestion runs-on: ubuntu-24.04 ``` Flagging here since it's the only CI file touched: there is **no CI workflow that exercises any Dockerfile** (verified — zero `docker build` references under `.github/workflows/`). PR's verification is "run `docker build` locally". Future regressions will ship silently. Consider adding a build-only smoke job: ```yaml docker-build: runs-on: ubuntu-24.04 strategy: matrix: dockerfile: - hugegraph-pd/Dockerfile - hugegraph-store/Dockerfile - hugegraph-server/Dockerfile - hugegraph-server/Dockerfile-hstore steps: - uses: actions/checkout@v4 - uses: docker/setup-buildx-action@v3 - run: docker buildx build -f ${{ matrix.dockerfile }} --load . ``` ########## hugegraph-server/Dockerfile: ########## @@ -39,25 +46,18 @@ ENV JAVA_OPTS="-XX:+UnlockExperimentalVMOptions -XX:+UseContainerSupport -XX:Max HUGEGRAPH_HOME="hugegraph-server" \ STDOUT_MODE="true" -#COPY . /hugegraph/hugegraph-server WORKDIR /hugegraph-server/ -# 1. Install environment and init HugeGraph Sever -RUN set -x \ - && rm /var/lib/dpkg/info/libc-bin.* \ - && apt-get -q clean \ - && apt-get -q update \ +# 1. Install runtime dependencies and configure server +RUN apt-get -q update \ && apt-get -q install -y --no-install-recommends --no-install-suggests \ dumb-init \ procps \ curl \ lsof \ Review Comment: 🧹 Pre-existing issue (not a regression from this PR, but worth fixing while in the area): the `cron` package stays installed in the runtime image (~3 MB), but no cron daemon is ever started — `dumb-init` only execs `docker-entrypoint.sh`. This means `start-hugegraph.sh -m true` silently fails to fire its scheduled monitor job, even though `crontab_append` returns success. Either: - Drop `cron` entirely (saves ~3 MB and removes the false impression that monitor mode works), or - Start cron in `docker-entrypoint.sh` (`cron &` before the server start) so the feature actually works. Same applies to `Dockerfile-hstore` and the PD / Store Dockerfiles. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
