snazy commented on code in PR #1:
URL: https://github.com/apache/polaris-tools/pull/1#discussion_r2020639235


##########
gradle/wrapper/gradle-wrapper.jar:
##########


Review Comment:
   The gradle-wrapper.jar should be removed. Binary files, especially those 
having code, are not good practice.
   There's a mechanism in the main repo to work around that.



##########
.github/workflows/main.yml:
##########
@@ -0,0 +1,64 @@
+#
+# 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.
+#
+
+name: CI
+
+on:
+  push:
+    branches: [ main ]
+  pull_request:
+
+jobs:
+  java:
+    name: Java/Gradle
+    runs-on: ubuntu-24.04
+    strategy:
+      max-parallel: 4
+      matrix:
+        java-version: [21, 23]
+    steps:
+      - uses: actions/checkout@v4
+        with:
+          submodules: 'true'
+
+      - name: Set up JDK
+        uses: actions/setup-java@v4
+        with:
+          distribution: 'temurin'
+          java-version: |
+            21
+            ${{ matrix.java-version != '21' && matrix.java-version || '' }}
+
+      - name: Setup Gradle
+        uses: gradle/actions/setup-gradle@v4
+
+      - name: Build & Check
+        run: ./gradlew --rerun-tasks assemble ${{ env.ADDITIONAL_GRADLE_OPTS 
}} check publishToMavenLocal --scan

Review Comment:
   ```suggestion
           run: ./gradlew --rerun-tasks assemble ${{ env.ADDITIONAL_GRADLE_OPTS 
}} check publishToMavenLocal
   ```
   
   No scan yet - see https://github.com/apache/polaris/pull/475



##########
.gitignore:
##########
@@ -0,0 +1,74 @@
+
+### Java ###
+# Compiled class file
+*.class
+
+# Log file
+*.log
+
+# BlueJ files
+*.ctxt
+
+# Mobile Tools for Java (J2ME)
+.mtj.tmp/
+
+# Package Files #
+*.jar
+*.war
+*.nar
+*.ear
+*.zip
+*.tar.gz
+*.rar
+
+# virtual machine crash logs, see 
http://www.java.com/en/download/help/error_hotspot.xml
+hs_err_pid*
+
+#misc
+target/
+dependency-reduced-pom.xml
+*.patch
+*.DS_Store
+.DS_Store
+
+#intellij
+*.iml
+.idea
+*.ipr
+*.iws
+
+# vscode
+.vscode
+
+# node
+node_modules/
+ui/src/generated/
+
+# Eclipse IDE
+.classpath
+.factorypath
+.project
+.settings
+.checkstyle
+out/
+
+# gradle
+.gradle/
+build/
+gradle/wrapper/gradle-wrapper.jar

Review Comment:
   Unresolved - see my other comment



##########
gradlew:
##########
@@ -0,0 +1,251 @@
+#!/bin/sh
+
+#
+# Copyright © 2015-2021 the original authors.
+#
+# Licensed 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
+#
+#      https://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.
+#
+# SPDX-License-Identifier: Apache-2.0
+#
+
+##############################################################################
+#
+#   Gradle start up script for POSIX generated by Gradle.
+#
+#   Important for running:
+#
+#   (1) You need a POSIX-compliant shell to run this script. If your /bin/sh is
+#       noncompliant, but you have some other compliant shell such as ksh or
+#       bash, then to run this script, type that shell name before the whole
+#       command line, like:
+#
+#           ksh Gradle
+#
+#       Busybox and similar reduced shells will NOT work, because this script
+#       requires all of these POSIX shell features:
+#         * functions;
+#         * expansions «$var», «${var}», «${var:-default}», «${var+SET}»,
+#           «${var#prefix}», «${var%suffix}», and «$( cmd )»;
+#         * compound commands having a testable exit status, especially «case»;
+#         * various built-in commands including «command», «set», and «ulimit».
+#
+#   Important for patching:
+#
+#   (2) This script targets any POSIX shell, so it avoids extensions provided
+#       by Bash, Ksh, etc; in particular arrays are avoided.
+#
+#       The "traditional" practice of packing multiple parameters into a
+#       space-separated string is a well documented source of bugs and security
+#       problems, so this is (mostly) avoided, by progressively accumulating
+#       options in "$@", and eventually passing that to Java.
+#
+#       Where the inherited environment variables (DEFAULT_JVM_OPTS, JAVA_OPTS,
+#       and GRADLE_OPTS) rely on word-splitting, this is performed explicitly;
+#       see the in-line comments for details.
+#
+#       There are tweaks for specific operating systems such as AIX, CygWin,
+#       Darwin, MinGW, and NonStop.
+#
+#   (3) This script is generated from the Groovy template
+#       
https://github.com/gradle/gradle/blob/HEAD/platforms/jvm/plugins-application/src/main/resources/org/gradle/api/internal/plugins/unixStartScript.txt
+#       within the Gradle project.
+#
+#       You can find Gradle at https://github.com/gradle/gradle/.
+#
+##############################################################################
+
+# Attempt to set APP_HOME
+
+# Resolve links: $0 may be a link
+app_path=$0
+
+# Need this for daisy-chained symlinks.
+while
+    APP_HOME=${app_path%"${app_path##*/}"}  # leaves a trailing /; empty if no 
leading path
+    [ -h "$app_path" ]
+do
+    ls=$( ls -ld "$app_path" )
+    link=${ls#*' -> '}
+    case $link in             #(
+      /*)   app_path=$link ;; #(
+      *)    app_path=$APP_HOME$link ;;
+    esac
+done
+
+# This is normally unused
+# shellcheck disable=SC2034
+APP_BASE_NAME=${0##*/}
+# Discard cd standard output in case $CDPATH is set 
(https://github.com/gradle/gradle/issues/25036)
+APP_HOME=$( cd -P "${APP_HOME:-./}" > /dev/null && printf '%s\n' "$PWD" ) || 
exit
+

Review Comment:
   See 
https://github.com/apache/polaris/blob/39cbcfb2d422f81992be7decfa6ba837583ba785/gradlew#L91



##########
NOTICE:
##########
@@ -0,0 +1,10 @@
+Apache Polaris (incubating)
+Copyright 2025 The Apache Software Foundation
+
+This product includes software developed at
+The Apache Software Foundation (http://www.apache.org/).
+
+The initial code for the Polaris project was donated
+to the ASF by Snowflake Inc. (https://www.snowflake.com/) copyright 2024.

Review Comment:
   This needs to be adopted



##########
gradlew.bat:
##########
@@ -0,0 +1,94 @@
+@rem

Review Comment:
   The gradlew.bat file needs to be removed to work with the 
non-gradle-wrapper-jar mechanism



##########
.github/workflows/main.yml:
##########
@@ -0,0 +1,64 @@
+#
+# 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.
+#
+
+name: CI
+
+on:
+  push:
+    branches: [ main ]
+  pull_request:
+
+jobs:
+  java:
+    name: Java/Gradle
+    runs-on: ubuntu-24.04
+    strategy:
+      max-parallel: 4
+      matrix:
+        java-version: [21, 23]
+    steps:
+      - uses: actions/checkout@v4
+        with:
+          submodules: 'true'
+
+      - name: Set up JDK
+        uses: actions/setup-java@v4
+        with:
+          distribution: 'temurin'
+          java-version: |
+            21
+            ${{ matrix.java-version != '21' && matrix.java-version || '' }}
+
+      - name: Setup Gradle
+        uses: gradle/actions/setup-gradle@v4
+
+      - name: Build & Check
+        run: ./gradlew --rerun-tasks assemble ${{ env.ADDITIONAL_GRADLE_OPTS 
}} check publishToMavenLocal --scan
+        # since the `nessieQuarkusApp` gradle plugin expects the below variable
+        env:
+          JDK17_HOME: ${{ env.JAVA_HOME_17_X64 }}

Review Comment:
   This should be superfluous



-- 
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: issues-unsubscr...@polaris.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Reply via email to