keith-turner commented on code in PR #19: URL: https://github.com/apache/accumulo-access/pull/19#discussion_r1342754628
########## .github/CONTRIBUTING.md: ########## @@ -0,0 +1,41 @@ +<!-- + + 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 + + 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. + +--> + +# Contributors Guide + +If you believe that you have found a [bug](https://github.com/apache/accumulo-access/labels/bug), please search for an existing [issue](https://github.com/apache/accumulo-access/issues) to see if it has already been reported. If you would like to add a new feature to Accumulo-Access, please send an email with your idea to the [dev](mailto:[email protected]) mail list. If it's appropriate, then we will create an issue. For simple changes, it's ok to just submit a pull request without an issue. + +## Development + +- See the [Developer's Guide](https://accumulo.apache.org/how-to-contribute/) for information regarding common build commands, IDE setup and more. +- Ensure that your work targets the correct branch +- Add/update unit and integration tests + +## Patch Submission + +- Ensure that Accumulo-Access builds cleanly before submitting your patch using the command: `mvn clean verify -DskipITs` Review Comment: ```suggestion - Ensure that Accumulo-Access builds cleanly before submitting your patch using the command: `mvn clean verify` ``` ########## pom.xml: ########## @@ -159,4 +191,240 @@ </plugin> </plugins> </build> + <profiles> + <profile> + <!-- This profile skips all Quality Assurance checks; activate with -PskipQA OR -DskipQA --> + <id>skipQA</id> + <activation> + <property> + <name>skipQA</name> + </property> + </activation> + <properties> + <accumulo.skip>true</accumulo.skip> + <apilyzer.skip>true</apilyzer.skip> + <checkstyle.skip>true</checkstyle.skip> + <formatter.skip>true</formatter.skip> + <impsort.skip>true</impsort.skip> + <mdep.analyze.skip>true</mdep.analyze.skip> + <modernizer.skip>true</modernizer.skip> + <rat.skip>true</rat.skip> + <skipITs>true</skipITs> + <skipTests>true</skipTests> + <sort.skip>true</sort.skip> + <spotbugs.skip>true</spotbugs.skip> + <warbucks.skip>true</warbucks.skip> + </properties> + </profile> + <profile> + <!-- off by default, but enable with '-P verifyformat' or '-DverifyFormat' --> + <id>verifyformat</id> + <activation> + <property> + <name>verifyFormat</name> + </property> + </activation> + <build> + <plugins> + <plugin> + <groupId>com.github.ekryd.sortpom</groupId> + <artifactId>sortpom-maven-plugin</artifactId> + <executions> + <execution> + <id>verify-sorted-pom</id> + <goals> + <goal>verify</goal> + </goals> + <phase>process-resources</phase> + </execution> + </executions> + </plugin> + <plugin> + <groupId>com.mycila</groupId> + <artifactId>license-maven-plugin</artifactId> + <executions> + <execution> + <id>verify-license-headers</id> + <goals> + <goal>check</goal> + </goals> + <phase>process-test-resources</phase> + </execution> + </executions> + </plugin> + <plugin> + <groupId>net.revelc.code.formatter</groupId> + <artifactId>formatter-maven-plugin</artifactId> + <executions> + <execution> + <id>verify-formatted-java-source</id> + <goals> + <goal>validate</goal> + </goals> + </execution> + </executions> + </plugin> + <plugin> + <groupId>net.revelc.code</groupId> + <artifactId>impsort-maven-plugin</artifactId> + <executions> + <execution> + <id>verify-sorted-imports</id> + <goals> + <goal>check</goal> + </goals> + </execution> + </executions> + </plugin> + </plugins> + </build> + </profile> + <profile> + <!-- on by default, but disable with '-P !autoformat' or '-DskipFormat' --> + <id>autoformat</id> + <activation> + <property> + <name>!skipFormat</name> + </property> + </activation> + <build> + <plugins> + <plugin> + <groupId>com.github.ekryd.sortpom</groupId> + <artifactId>sortpom-maven-plugin</artifactId> + <executions> + <execution> + <id>sort-pom</id> + <goals> + <goal>sort</goal> + </goals> + <phase>process-sources</phase> + </execution> + </executions> + </plugin> + <plugin> + <groupId>com.mycila</groupId> + <artifactId>license-maven-plugin</artifactId> + <executions> + <execution> + <id>license-headers</id> + <goals> + <goal>format</goal> + </goals> + <phase>process-test-resources</phase> + </execution> + </executions> + </plugin> + <plugin> + <groupId>net.revelc.code.formatter</groupId> + <artifactId>formatter-maven-plugin</artifactId> + <executions> + <execution> + <id>format-java-source</id> + <goals> + <goal>format</goal> + </goals> + </execution> + </executions> + </plugin> + <plugin> + <groupId>net.revelc.code</groupId> + <artifactId>impsort-maven-plugin</artifactId> + <executions> + <execution> + <id>sort-imports</id> + <goals> + <goal>sort</goal> + </goals> + </execution> + </executions> + </plugin> + </plugins> + </build> + </profile> + <profile> + <id>sec-bugs</id> + <build> + <plugins> + <plugin> + <groupId>com.github.spotbugs</groupId> + <artifactId>spotbugs-maven-plugin</artifactId> + <configuration> + <plugins> + <plugin> + <groupId>com.h3xstream.findsecbugs</groupId> + <artifactId>findsecbugs-plugin</artifactId> + <version>1.12.0</version> + </plugin> + </plugins> + </configuration> + </plugin> + </plugins> + </build> + </profile> + <profile> + <!-- This profile uses the Google ErrorProne tool to perform static code analysis at + compile time. Auto-generated code is not checked. + See: https://errorprone.info/bugpatterns for list of available bug patterns.--> + <id>errorprone</id> + <build> + <plugins> + <plugin> + <groupId>org.apache.maven.plugins</groupId> + <artifactId>maven-compiler-plugin</artifactId> + <configuration> + <compilerArgs> + <arg>-XDcompilePolicy=simple</arg> + <arg> + -Xplugin:ErrorProne \ + -XepExcludedPaths:.*/(thrift|generated-sources|src/test)/.* \ Review Comment: Some of these excludes could be removed ########## .github/workflows/maven.yaml: ########## @@ -0,0 +1,133 @@ +# +# 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 +# +# 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. +# + +# This workflow will build a Java project with Maven +# See also: +# https://help.github.com/actions/language-and-framework-guides/building-and-testing-java-with-maven + +name: QA + +on: + push: + branches: [ '*' ] + pull_request: + branches: [ '*' ] + +permissions: + contents: read + +jobs: + # fast build to populate the local maven repository cache + fastbuild: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v3 + - name: Set up JDK 11 + uses: actions/setup-java@v3 + with: + distribution: adopt + java-version: 11 + - name: Cache local maven repository + uses: actions/cache@v3 + with: + path: | + ~/.m2/repository/ + !~/.m2/repository/org/apache/accumulo + key: ${{ runner.os }}-m2-${{ hashFiles('**/pom.xml') }} + restore-keys: ${{ runner.os }}-m2 + - name: Show the first log message + run: git log -n1 + - name: Check for unapproved characters + run: src/build/ci/find-unapproved-chars.sh + - name: Check for unapproved JUnit API usage + run: src/build/ci/find-unapproved-junit.sh + - name: Build with Maven (Fast Build) + timeout-minutes: 20 + run: mvn -B -V -e -ntp "-Dstyle.color=always" clean package dependency:resolve -DskipTests -DskipFormat -DverifyFormat + env: + MAVEN_OPTS: -Djansi.force=true + # more complete builds with tests + mvn: + needs: fastbuild + strategy: + matrix: + profile: + - {name: 'unit-tests', javaver: 11, args: 'verify -PskipQA -DskipTests=false'} Review Comment: The builds for accumulo-access are so much faster than the builds for accumulo, could have a simpler matrix. Maybe just two builds, one with java 11 and another with java 17. Not sure if the javadoc build could be added to one of those or if a third build is needed for it. Also can drop `-DskipITs` ########## src/main/java/org/apache/accumulo/access/AeNode.java: ########## @@ -1,3 +1,21 @@ +/* Review Comment: Were some of these licenses automatically added by a plugin you added to the pom? ########## src/build/ci/find-unapproved-chars.sh: ########## @@ -0,0 +1,52 @@ +#! /usr/bin/env bash +# +# 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 +# +# 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. +# + +# The purpose of this ci script is to ensure that a pull request doesn't +# unintentionally, or maliciously, add any new non-ASCII characters unless they +# are preapproved on the ALLOWED list or in known binary or resource files +NUM_EXPECTED=0 +ALLOWED='©èö🐈三四五六八九十' + +function findallnonascii() { + # -P for perl matching, -o for only showing the match for counting occurrences not lines + local opts='-Po' + if [[ $1 == 'print' ]]; then + # -P for perl matching, -H for always showing filenames, -n for showing line numbers + opts='-PHn' + fi + find . -type f \ + -not -path '*/\.git/*' \ + -not -path '*/monitor/resources/external/*' \ + -not -path '*/tserver/src/test/resources/walog-from-14/*' \ Review Comment: These exclusions could be removed ########## src/build/ci/install-shfmt.sh: ########## @@ -0,0 +1,29 @@ +#! /usr/bin/env bash +# +# 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 +# +# 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. +# + +# Install shfmt tool to search for and optionally format bash scripts +# This is useful for other CI tools to run ShellCheck and shfmt to format + +set -e +set -x + +shfmt_version=3.4.3 +sudo wget "https://github.com/mvdan/sh/releases/download/v${shfmt_version}/shfmt_v${shfmt_version}_linux_amd64"; -O /usr/local/bin/shfmt && Review Comment: Would be safer to check for a known hash of this after downloading. This should probably be done in accumulo first. ########## .github/workflows/maven.yaml: ########## @@ -0,0 +1,133 @@ +# +# 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 +# +# 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. +# + +# This workflow will build a Java project with Maven +# See also: +# https://help.github.com/actions/language-and-framework-guides/building-and-testing-java-with-maven + +name: QA + +on: + push: + branches: [ '*' ] + pull_request: + branches: [ '*' ] + +permissions: + contents: read + +jobs: + # fast build to populate the local maven repository cache + fastbuild: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v3 + - name: Set up JDK 11 + uses: actions/setup-java@v3 + with: + distribution: adopt + java-version: 11 + - name: Cache local maven repository + uses: actions/cache@v3 + with: + path: | + ~/.m2/repository/ + !~/.m2/repository/org/apache/accumulo + key: ${{ runner.os }}-m2-${{ hashFiles('**/pom.xml') }} + restore-keys: ${{ runner.os }}-m2 + - name: Show the first log message + run: git log -n1 + - name: Check for unapproved characters + run: src/build/ci/find-unapproved-chars.sh + - name: Check for unapproved JUnit API usage + run: src/build/ci/find-unapproved-junit.sh + - name: Build with Maven (Fast Build) + timeout-minutes: 20 + run: mvn -B -V -e -ntp "-Dstyle.color=always" clean package dependency:resolve -DskipTests -DskipFormat -DverifyFormat + env: + MAVEN_OPTS: -Djansi.force=true + # more complete builds with tests + mvn: + needs: fastbuild + strategy: + matrix: + profile: + - {name: 'unit-tests', javaver: 11, args: 'verify -PskipQA -DskipTests=false'} + - {name: 'qa-checks', javaver: 11, args: 'verify javadoc:jar -Psec-bugs -DskipTests -Dspotbugs.timeout=3600000'} + - {name: 'errorprone', javaver: 11, args: 'verify -Perrorprone,skipQA'} + - {name: 'jdk17', javaver: 17, args: 'verify -DskipITs'} + fail-fast: false + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v3 + - name: Set up JDK ${{ matrix.profile.javaver }} + uses: actions/setup-java@v3 + with: + distribution: adopt + java-version: ${{ matrix.profile.javaver }} + - name: Cache local maven repository + uses: actions/cache@v3 + witi: + path: | + ~/.m2/repository/ + !~/.m2/repository/org/apache/accumulo + key: ${{ runner.os }}-m2-${{ hashFiles('**/pom.xml') }} + restore-keys: ${{ runner.os }}-m2 + - name: Override DNS to fix IP address for hostname Review Comment: I wonder if this was added so that the mini test could run, may be able to drop this section ########## src/build/ci/find-unapproved-chars.sh: ########## @@ -0,0 +1,52 @@ +#! /usr/bin/env bash +# +# 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 +# +# 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. +# + +# The purpose of this ci script is to ensure that a pull request doesn't +# unintentionally, or maliciously, add any new non-ASCII characters unless they +# are preapproved on the ALLOWED list or in known binary or resource files +NUM_EXPECTED=0 +ALLOWED='©èö🐈三四五六八九十' + +function findallnonascii() { + # -P for perl matching, -o for only showing the match for counting occurrences not lines + local opts='-Po' + if [[ $1 == 'print' ]]; then + # -P for perl matching, -H for always showing filenames, -n for showing line numbers + opts='-PHn' + fi + find . -type f \ + -not -path '*/\.git/*' \ + -not -path '*/monitor/resources/external/*' \ + -not -path '*/tserver/src/test/resources/walog-from-14/*' \ + -not -regex '.*[.]\(png\|jar\|rf\|jceks\|walog\)$' \ Review Comment: could probably remove the walog and rf exclusion ########## src/build/ci/find-unapproved-chars.sh: ########## @@ -0,0 +1,52 @@ +#! /usr/bin/env bash Review Comment: It seems like a good check to me. The only pitfall of having it is that is has to maintained. The script has been fairly stable in accumulo, so may not be much to do in the way of maintenance. -- 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]
