FlorianHockmann commented on code in PR #1788: URL: https://github.com/apache/tinkerpop/pull/1788#discussion_r953518537
########## gremlin-dotnet/docker-compose.yml: ########## @@ -0,0 +1,56 @@ +# 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. + +version: "3.7" Review Comment: (nitpick) Could be updated to 3.8 which is currently the latest version. ########## gremlin-dotnet/test/pom.xml: ########## @@ -62,52 +68,64 @@ limitations under the License. </build> <profiles> + <!-- Test gremlin-dotnet javascript in Docker --> <profile> - <id>gremlin-dotnet-standard</id> - <activation> - <activeByDefault>true</activeByDefault> - </activation> - <properties> - <packaging.type>pom</packaging.type> - </properties> - </profile> - <!-- activates the building of .NET components and requires that the .NET SDK be installed on the system --> - <profile> - <id>gremlin-dotnet</id> + <id>glv-dotnet</id> Review Comment: Does this need to be renamed? We have the profile name in the [dev docs](https://tinkerpop.apache.org/docs/current/dev/developer/#dotnet-environment) and it's used in GHA. Contributors also need to know about this renaming so they use the new name. Otherwise their build will suddenly omit the GLV part which they might not even notice (assuming that the `.glv` files aren't used which is another option instead of activating the profile manually). ########## .github/workflows/build-test.yml: ########## @@ -141,20 +174,41 @@ jobs: javascript: name: javascript timeout-minutes: 15 - needs: smoke + needs: cache-gremlin-server-docker-image runs-on: ${{ matrix.os }} strategy: matrix: - os: [ubuntu-latest, windows-latest] + # Windows Disabled until Linux containers are supported on Windows runners: https://github.com/actions/virtual-environments/issues/252 + # os: [ubuntu-latest, windows-latest] + os: [ubuntu-latest] steps: - uses: actions/checkout@v3 - name: Set up JDK 11 uses: actions/setup-java@v3 with: java-version: '11' distribution: 'temurin' + - name: Get Cached Server Base Image + if: matrix.os == 'ubuntu-latest' + uses: actions/cache@v3 + id: gremlin-server-test-docker-image + with: + path: | + ./gremlin-server/* + ~/.m2/repository/org/apache/tinkerpop/* + key: ${{ github.sha }} + - name: Download Server Base Image + if: matrix.os == 'windows-latest' Review Comment: (nitpick) Since you have commented out the `windows-latest` completely above in line 182, I suggest this step can also be commented out completely. ########## gremlin-dotnet/docker-compose.yml: ########## @@ -0,0 +1,56 @@ +# 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. + +version: "3.7" + +services: + + gremlin-server-test-dotnet: + container_name: gremlin-server-test-dotnet + image: tinkerpop:gremlin-server-test-${GREMLIN_SERVER} + build: + context: ../ + dockerfile: docker/gremlin-test-server/Dockerfile + args: + - GREMLIN_SERVER=${GREMLIN_SERVER} + ports: + - "45940:45940" + - "45941:45941" + - "45942:45942" + - "4588:4588" + volumes: + - ${HOME}/.groovy:/root/.groovy + - ${HOME}/.m2:/root/.m2 + - ${ABS_PROJECT_HOME}/gremlin-test/target:/opt/gremlin-test + + gremlin-dotnet-integration-tests: + container_name: gremlin-dotnet-integration-tests + image: mcr.microsoft.com/dotnet/sdk:6.0 + volumes: + - .:/gremlin-dotnet + - ../gremlin-test/features:/gremlin-test/features + - ../docker/gremlin-test-server:/gremlin-dotnet/gremlin-test-server + environment: + - DOCKER_ENVIRONMENT=true + - TEST_TRANSACTIONS=true + working_dir: /gremlin-dotnet + command: > + bash -c "apt-get update && apt-get install dos2unix && dos2unix ./gremlin-test-server/wait-for-server.sh + && ./gremlin-test-server/wait-for-server.sh gremlin-server-test-dotnet 45940 300 + && dotnet test ./Gremlin.Net.sln" + depends_on: Review Comment: It might be possible to get rid of the `wait-for-server.sh` if we use the [_long syntax_ of `depends_on`](https://docs.docker.com/compose/compose-file/#long-syntax-1) which supports the `condition: service_healthy`. Then we would only need a healthcheck for the test Gremlin Server. But that is just something that we could maybe improve in the future. The solution in this PR is definitely good enough from my side. I just wanted to share this idea. ########## gremlin-dotnet/docker-compose.yml: ########## @@ -0,0 +1,56 @@ +# 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. + +version: "3.7" + +services: + + gremlin-server-test-dotnet: + container_name: gremlin-server-test-dotnet + image: tinkerpop:gremlin-server-test-${GREMLIN_SERVER} Review Comment: (nitpick) I would change this to: `tinkerpop/gremlin-server-test:${GREMLIN_SERVER}` to stay consistent with our current naming scheme. ########## .github/workflows/build-test.yml: ########## @@ -141,20 +174,41 @@ jobs: javascript: name: javascript timeout-minutes: 15 - needs: smoke + needs: cache-gremlin-server-docker-image runs-on: ${{ matrix.os }} strategy: matrix: - os: [ubuntu-latest, windows-latest] + # Windows Disabled until Linux containers are supported on Windows runners: https://github.com/actions/virtual-environments/issues/252 Review Comment: Hmm, this is really unfortunate. The issue was closed as `wontfix` in 2020 and there hasn't been any progress since then. So, it doesn't seem likely that this will be supported any time soon. That means of course that we might accidentally break the Windows build without noticing it until someone executes the tests locally on their Windows machine. This is a big step back after we just got support for running these tests also on Windows via GHA, but I guess simplifying the build environment in general for contributors is more important and therefore justifies this. @xiazcy also already mentioned that this will likely be necessary [on the dev list](https://lists.apache.org/thread/yytdsgyl6rgso0dkzorlbk4nhb61jblo) and nobody objected. We could maybe at some point in the future look into whether we want to add another CI provider that supports Linux containers on Windows or whether we could even create a Gremlin Server Windows container. But that's definitely out of scope for this PR. ########## docker/gremlin-server/gremlin-server-integration.yaml: ########## @@ -18,13 +18,15 @@ host: 0.0.0.0 port: 45940 evaluationTimeout: 30000 +channelizer: org.apache.tinkerpop.gremlin.server.channel.WsAndHttpChannelizer graphs: { - graph: conf/tinkergraph-empty.properties, - classic: conf/tinkergraph-empty.properties, - modern: conf/tinkergraph-empty.properties, - crew: conf/tinkergraph-empty.properties, - grateful: conf/tinkergraph-empty.properties, - sink: conf/tinkergraph-empty.properties + graph: scripts/tinkergraph-empty.properties, Review Comment: It's not that important, but I'm just wondering: Why are these config files now under `scripts` and not `conf`? ########## gremlin-dotnet/test/pom.xml: ########## @@ -161,192 +179,10 @@ limitations under the License. </scripts> </configuration> </execution> - <execution> - <id>gremlin-server-start</id> - <phase>pre-integration-test</phase> - <goals> - <goal>execute</goal> - </goals> - <configuration> - <properties> - <property> - <name>skipTests</name> - <value>${skipTests}</value> - </property> - <property> - <name>gremlinServerDir</name> - <value>${gremlin.server.dir}</value> - </property> - <property> - <name>settingsFile</name> - <value>${gremlin.server.dir}/src/test/resources/org/apache/tinkerpop/gremlin/server/gremlin-server-integration.yaml</value> - </property> - <property> - <name>executionName</name> - <value>${project.name}</value> - </property> - <property> - <name>projectBaseDir</name> - <value>${project.basedir}</value> - </property> - <property> - <name>tinkerpopRootDir</name> - <value>${tinkerpop.root.dir}</value> - </property> - </properties> - <scripts> - <script>${gremlin.server.dir}/src/test/scripts/test-server-start.groovy</script> - </scripts> - </configuration> - </execution> - <execution> - <id>gremlin-server-stop</id> - <phase>post-integration-test</phase> - <goals> - <goal>execute</goal> - </goals> - <configuration> - <properties> - <property> - <name>skipTests</name> - <value>${skipTests}</value> - </property> - <property> - <name>executionName</name> - <value>${project.name}</value> - </property> - </properties> - <scripts> - <script>${gremlin.server.dir}/src/test/scripts/test-server-stop.groovy</script> - </scripts> - </configuration> - </execution> </executions> </plugin> </plugins> </build> </profile> - <!-- - This profile will include neo4j for purposes of transactional testing within Gremlin Server. - Tests that require neo4j specifically will be "ignored" if this profile is not turned on. - --> - <profile> - <id>include-neo4j</id> - <activation> - <activeByDefault>false</activeByDefault> - <property> - <name>includeNeo4j</name> Review Comment: This property is still used in the GHA job. Please remove it there as it doesn't work any more as these tests are now enabled by default. (Or add support for this property back instead of always executing these tests, but I don't have a strong preference here about whether that's necessary or not.) ########## gremlin-dotnet/test/pom.xml: ########## @@ -62,52 +68,64 @@ limitations under the License. </build> <profiles> + <!-- Test gremlin-dotnet javascript in Docker --> Review Comment: ```suggestion <!-- Test gremlin-dotnet in Docker --> ``` -- 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]
