Copilot commented on code in PR #6326:
URL: https://github.com/apache/shenyu/pull/6326#discussion_r3083475889
##########
.github/workflows/integrated-test-k8s-ingress.yml:
##########
@@ -60,155 +56,201 @@ jobs:
- '!.github/ISSUE_TEMPLATE/**'
- '!.github/PULL_REQUEST_TEMPLATE'
+ prepare-images:
+ needs: changes
+ if: github.repository == 'apache/shenyu' &&
needs.changes.outputs.k8s-ingress == 'true'
+ runs-on: ubuntu-latest
+ steps:
+ - uses: actions/checkout@v3
+ with:
+ submodules: true
+
- name: Clean Space
- if: steps.filter.outputs.k8s-ingress == 'true'
run: |
sudo rm -rf /usr/share/dotnet
sudo rm -rf /opt/ghc
sudo rm -rf "/usr/local/share/boost"
sudo rm -rf "$AGENT_TOOLSDIRECTORY"
- - name: Cache Maven Repos
- if: steps.filter.outputs.k8s-ingress == 'true'
- uses: actions/cache@v3
- with:
- path: ~/.m2/repository
- key: ${{ runner.os }}-maven-${{ hashFiles('**/pom.xml') }}
- restore-keys: |
- ${{ runner.os }}-maven-
-
- - uses: actions/setup-java@v4
- if: steps.filter.outputs.k8s-ingress == 'true'
- with:
- java-version: 17
- distribution: "temurin"
-
- - name: Install Go
- uses: actions/setup-go@v3
- with:
- go-version: 1.17.x
-
- - name: Install k8s
- if: steps.filter.outputs.k8s-ingress == 'true'
- run: |
- go install sigs.k8s.io/[email protected]
- curl -LO
https://storage.googleapis.com/kubernetes-release/release/v1.24.14/bin/linux/amd64/kubectl
&& sudo install kubectl /usr/local/bin/kubectl
- kind create cluster --image=kindest/node:v1.21.1
--config=./shenyu-integrated-test/${{ matrix.case }}/deploy/kind-config.yaml
-
- - name: Cache Maven Repos
- if: steps.filter.outputs.k8s-ingress == 'true'
- uses: actions/cache@v3
+ - name: Restore Maven Repos
+ id: restore-maven-cache
+ uses: actions/cache/restore@v3
with:
path: ~/.m2/repository
key: ${{ runner.os }}-maven-${{ hashFiles('**/pom.xml') }}
restore-keys: |
${{ runner.os }}-maven-
- uses: actions/setup-java@v4
- if: steps.filter.outputs.k8s-ingress == 'true'
with:
java-version: 17
distribution: "temurin"
- name: Install mvnd
- if: steps.filter.outputs.k8s-ingress == 'true'
shell: bash
run: |
MVND_VERSION=1.0.2
- if [[ "${{ runner.os }}" == "Windows" ]]; then
- curl -sL
https://downloads.apache.org/maven/mvnd/${MVND_VERSION}/maven-mvnd-${MVND_VERSION}-windows-amd64.zip
-o mvnd.zip
- unzip -q mvnd.zip
- mkdir -p $HOME/.local
- mv maven-mvnd-${MVND_VERSION}-windows-amd64 $HOME/.local/mvnd
- echo "$HOME/.local/mvnd/bin" >> $GITHUB_PATH
- echo "MVND_HOME=$HOME/.local/mvnd" >> $GITHUB_ENV
- else
- curl -sL
https://downloads.apache.org/maven/mvnd/${MVND_VERSION}/maven-mvnd-${MVND_VERSION}-linux-amd64.zip
-o mvnd.zip
- unzip -q mvnd.zip
- mkdir -p $HOME/.local
- mv maven-mvnd-${MVND_VERSION}-linux-amd64 $HOME/.local/mvnd
- echo "$HOME/.local/mvnd/bin" >> $GITHUB_PATH
- echo "MVND_HOME=$HOME/.local/mvnd" >> $GITHUB_ENV
- fi
+ curl -sL
https://downloads.apache.org/maven/mvnd/${MVND_VERSION}/maven-mvnd-${MVND_VERSION}-linux-amd64.zip
-o mvnd.zip
+ unzip -q mvnd.zip
+ mkdir -p $HOME/.local
+ mv maven-mvnd-${MVND_VERSION}-linux-amd64 $HOME/.local/mvnd
+ echo "$HOME/.local/mvnd/bin" >> $GITHUB_PATH
+ echo "MVND_HOME=$HOME/.local/mvnd" >> $GITHUB_ENV
- name: Build with Maven
- if: steps.filter.outputs.k8s-ingress == 'true'
shell: bash
run: |
if mvnd --version > /dev/null 2>&1; then
echo "Using mvnd for build"
mvnd -B clean install -Dmaven.javadoc.skip=true
-Dmaven.test.skip=true
else
echo "Falling back to maven wrapper"
- if [[ "${{ runner.os }}" == "Windows" ]]; then
- ./mvnw.cmd -B clean install -Dmaven.javadoc.skip=true
-Dmaven.test.skip=true
- else
- ./mvnw -B clean install -Dmaven.javadoc.skip=true
-Dmaven.test.skip=true
- fi
+ ./mvnw -B clean install -Dmaven.javadoc.skip=true
-Dmaven.test.skip=true
fi
- name: Build integrated tests
- if: steps.filter.outputs.k8s-ingress == 'true'
shell: bash
run: |
if mvnd --version > /dev/null 2>&1; then
echo "Using mvnd for build integrated tests"
mvnd -B clean install -Pit -DskipTests -am -f
./shenyu-integrated-test/pom.xml
else
echo "Falling back to maven wrapper for integrated tests"
- if [[ "${{ runner.os }}" == "Windows" ]]; then
- ./mvnw.cmd -B clean install -Pit -DskipTests -am -f
./shenyu-integrated-test/pom.xml
- else
- ./mvnw -B clean install -Pit -DskipTests -am -f
./shenyu-integrated-test/pom.xml
- fi
+ ./mvnw -B clean install -Pit -DskipTests -am -f
./shenyu-integrated-test/pom.xml
fi
- name: Build examples
- if: steps.filter.outputs.k8s-ingress == 'true'
shell: bash
run: |
if mvnd --version > /dev/null 2>&1; then
echo "Using mvnd for build examples"
mvnd -B clean install -Pexample -Dmaven.javadoc.skip=true
-Dmaven.test.skip=true -am -f ./shenyu-examples/pom.xml
else
echo "Falling back to maven wrapper for examples"
- if [[ "${{ runner.os }}" == "Windows" ]]; then
- ./mvnw.cmd -B clean install -Pexample -Dmaven.javadoc.skip=true
-Dmaven.test.skip=true -am -f ./shenyu-examples/pom.xml
- else
- ./mvnw -B clean install -Pexample -Dmaven.javadoc.skip=true
-Dmaven.test.skip=true -am -f ./shenyu-examples/pom.xml
- fi
+ ./mvnw -B clean install -Pexample -Dmaven.javadoc.skip=true
-Dmaven.test.skip=true -am -f ./shenyu-examples/pom.xml
fi
Review Comment:
This `Build examples` step runs `clean install` with `-am`, which will
trigger additional cleaning/rebuilding of dependencies and increases runtime.
Prefer running without `clean` (or merging with the prior Maven invocation) so
the example image build can reuse already-built artifacts.
##########
.github/workflows/integrated-test-k8s-ingress.yml:
##########
@@ -60,155 +56,201 @@ jobs:
- '!.github/ISSUE_TEMPLATE/**'
- '!.github/PULL_REQUEST_TEMPLATE'
+ prepare-images:
+ needs: changes
+ if: github.repository == 'apache/shenyu' &&
needs.changes.outputs.k8s-ingress == 'true'
+ runs-on: ubuntu-latest
+ steps:
+ - uses: actions/checkout@v3
+ with:
+ submodules: true
+
- name: Clean Space
- if: steps.filter.outputs.k8s-ingress == 'true'
run: |
sudo rm -rf /usr/share/dotnet
sudo rm -rf /opt/ghc
sudo rm -rf "/usr/local/share/boost"
sudo rm -rf "$AGENT_TOOLSDIRECTORY"
- - name: Cache Maven Repos
- if: steps.filter.outputs.k8s-ingress == 'true'
- uses: actions/cache@v3
- with:
- path: ~/.m2/repository
- key: ${{ runner.os }}-maven-${{ hashFiles('**/pom.xml') }}
- restore-keys: |
- ${{ runner.os }}-maven-
-
- - uses: actions/setup-java@v4
- if: steps.filter.outputs.k8s-ingress == 'true'
- with:
- java-version: 17
- distribution: "temurin"
-
- - name: Install Go
- uses: actions/setup-go@v3
- with:
- go-version: 1.17.x
-
- - name: Install k8s
- if: steps.filter.outputs.k8s-ingress == 'true'
- run: |
- go install sigs.k8s.io/[email protected]
- curl -LO
https://storage.googleapis.com/kubernetes-release/release/v1.24.14/bin/linux/amd64/kubectl
&& sudo install kubectl /usr/local/bin/kubectl
- kind create cluster --image=kindest/node:v1.21.1
--config=./shenyu-integrated-test/${{ matrix.case }}/deploy/kind-config.yaml
-
- - name: Cache Maven Repos
- if: steps.filter.outputs.k8s-ingress == 'true'
- uses: actions/cache@v3
+ - name: Restore Maven Repos
+ id: restore-maven-cache
+ uses: actions/cache/restore@v3
with:
path: ~/.m2/repository
key: ${{ runner.os }}-maven-${{ hashFiles('**/pom.xml') }}
restore-keys: |
${{ runner.os }}-maven-
- uses: actions/setup-java@v4
- if: steps.filter.outputs.k8s-ingress == 'true'
with:
java-version: 17
distribution: "temurin"
- name: Install mvnd
- if: steps.filter.outputs.k8s-ingress == 'true'
shell: bash
run: |
MVND_VERSION=1.0.2
- if [[ "${{ runner.os }}" == "Windows" ]]; then
- curl -sL
https://downloads.apache.org/maven/mvnd/${MVND_VERSION}/maven-mvnd-${MVND_VERSION}-windows-amd64.zip
-o mvnd.zip
- unzip -q mvnd.zip
- mkdir -p $HOME/.local
- mv maven-mvnd-${MVND_VERSION}-windows-amd64 $HOME/.local/mvnd
- echo "$HOME/.local/mvnd/bin" >> $GITHUB_PATH
- echo "MVND_HOME=$HOME/.local/mvnd" >> $GITHUB_ENV
- else
- curl -sL
https://downloads.apache.org/maven/mvnd/${MVND_VERSION}/maven-mvnd-${MVND_VERSION}-linux-amd64.zip
-o mvnd.zip
- unzip -q mvnd.zip
- mkdir -p $HOME/.local
- mv maven-mvnd-${MVND_VERSION}-linux-amd64 $HOME/.local/mvnd
- echo "$HOME/.local/mvnd/bin" >> $GITHUB_PATH
- echo "MVND_HOME=$HOME/.local/mvnd" >> $GITHUB_ENV
- fi
+ curl -sL
https://downloads.apache.org/maven/mvnd/${MVND_VERSION}/maven-mvnd-${MVND_VERSION}-linux-amd64.zip
-o mvnd.zip
+ unzip -q mvnd.zip
+ mkdir -p $HOME/.local
+ mv maven-mvnd-${MVND_VERSION}-linux-amd64 $HOME/.local/mvnd
+ echo "$HOME/.local/mvnd/bin" >> $GITHUB_PATH
+ echo "MVND_HOME=$HOME/.local/mvnd" >> $GITHUB_ENV
- name: Build with Maven
- if: steps.filter.outputs.k8s-ingress == 'true'
shell: bash
run: |
if mvnd --version > /dev/null 2>&1; then
echo "Using mvnd for build"
mvnd -B clean install -Dmaven.javadoc.skip=true
-Dmaven.test.skip=true
else
echo "Falling back to maven wrapper"
- if [[ "${{ runner.os }}" == "Windows" ]]; then
- ./mvnw.cmd -B clean install -Dmaven.javadoc.skip=true
-Dmaven.test.skip=true
- else
- ./mvnw -B clean install -Dmaven.javadoc.skip=true
-Dmaven.test.skip=true
- fi
+ ./mvnw -B clean install -Dmaven.javadoc.skip=true
-Dmaven.test.skip=true
fi
Review Comment:
`prepare-images` runs three separate Maven builds, each starting with
`clean`, which forces redundant recompilation and reduces the CI savings from
deduplicating this workflow. Consider removing the initial full `clean install`
here and/or consolidating the needed modules/profiles into a single build so
dependencies are built once (e.g., avoid multiple `clean` invocations).
##########
.github/workflows/integrated-test-k8s-ingress.yml:
##########
@@ -60,155 +56,201 @@ jobs:
- '!.github/ISSUE_TEMPLATE/**'
- '!.github/PULL_REQUEST_TEMPLATE'
+ prepare-images:
+ needs: changes
+ if: github.repository == 'apache/shenyu' &&
needs.changes.outputs.k8s-ingress == 'true'
+ runs-on: ubuntu-latest
+ steps:
+ - uses: actions/checkout@v3
+ with:
+ submodules: true
+
- name: Clean Space
- if: steps.filter.outputs.k8s-ingress == 'true'
run: |
sudo rm -rf /usr/share/dotnet
sudo rm -rf /opt/ghc
sudo rm -rf "/usr/local/share/boost"
sudo rm -rf "$AGENT_TOOLSDIRECTORY"
- - name: Cache Maven Repos
- if: steps.filter.outputs.k8s-ingress == 'true'
- uses: actions/cache@v3
- with:
- path: ~/.m2/repository
- key: ${{ runner.os }}-maven-${{ hashFiles('**/pom.xml') }}
- restore-keys: |
- ${{ runner.os }}-maven-
-
- - uses: actions/setup-java@v4
- if: steps.filter.outputs.k8s-ingress == 'true'
- with:
- java-version: 17
- distribution: "temurin"
-
- - name: Install Go
- uses: actions/setup-go@v3
- with:
- go-version: 1.17.x
-
- - name: Install k8s
- if: steps.filter.outputs.k8s-ingress == 'true'
- run: |
- go install sigs.k8s.io/[email protected]
- curl -LO
https://storage.googleapis.com/kubernetes-release/release/v1.24.14/bin/linux/amd64/kubectl
&& sudo install kubectl /usr/local/bin/kubectl
- kind create cluster --image=kindest/node:v1.21.1
--config=./shenyu-integrated-test/${{ matrix.case }}/deploy/kind-config.yaml
-
- - name: Cache Maven Repos
- if: steps.filter.outputs.k8s-ingress == 'true'
- uses: actions/cache@v3
+ - name: Restore Maven Repos
+ id: restore-maven-cache
+ uses: actions/cache/restore@v3
with:
path: ~/.m2/repository
key: ${{ runner.os }}-maven-${{ hashFiles('**/pom.xml') }}
restore-keys: |
${{ runner.os }}-maven-
- uses: actions/setup-java@v4
- if: steps.filter.outputs.k8s-ingress == 'true'
with:
java-version: 17
distribution: "temurin"
- name: Install mvnd
- if: steps.filter.outputs.k8s-ingress == 'true'
shell: bash
run: |
MVND_VERSION=1.0.2
- if [[ "${{ runner.os }}" == "Windows" ]]; then
- curl -sL
https://downloads.apache.org/maven/mvnd/${MVND_VERSION}/maven-mvnd-${MVND_VERSION}-windows-amd64.zip
-o mvnd.zip
- unzip -q mvnd.zip
- mkdir -p $HOME/.local
- mv maven-mvnd-${MVND_VERSION}-windows-amd64 $HOME/.local/mvnd
- echo "$HOME/.local/mvnd/bin" >> $GITHUB_PATH
- echo "MVND_HOME=$HOME/.local/mvnd" >> $GITHUB_ENV
- else
- curl -sL
https://downloads.apache.org/maven/mvnd/${MVND_VERSION}/maven-mvnd-${MVND_VERSION}-linux-amd64.zip
-o mvnd.zip
- unzip -q mvnd.zip
- mkdir -p $HOME/.local
- mv maven-mvnd-${MVND_VERSION}-linux-amd64 $HOME/.local/mvnd
- echo "$HOME/.local/mvnd/bin" >> $GITHUB_PATH
- echo "MVND_HOME=$HOME/.local/mvnd" >> $GITHUB_ENV
- fi
+ curl -sL
https://downloads.apache.org/maven/mvnd/${MVND_VERSION}/maven-mvnd-${MVND_VERSION}-linux-amd64.zip
-o mvnd.zip
+ unzip -q mvnd.zip
+ mkdir -p $HOME/.local
+ mv maven-mvnd-${MVND_VERSION}-linux-amd64 $HOME/.local/mvnd
+ echo "$HOME/.local/mvnd/bin" >> $GITHUB_PATH
+ echo "MVND_HOME=$HOME/.local/mvnd" >> $GITHUB_ENV
- name: Build with Maven
- if: steps.filter.outputs.k8s-ingress == 'true'
shell: bash
run: |
if mvnd --version > /dev/null 2>&1; then
echo "Using mvnd for build"
mvnd -B clean install -Dmaven.javadoc.skip=true
-Dmaven.test.skip=true
else
echo "Falling back to maven wrapper"
- if [[ "${{ runner.os }}" == "Windows" ]]; then
- ./mvnw.cmd -B clean install -Dmaven.javadoc.skip=true
-Dmaven.test.skip=true
- else
- ./mvnw -B clean install -Dmaven.javadoc.skip=true
-Dmaven.test.skip=true
- fi
+ ./mvnw -B clean install -Dmaven.javadoc.skip=true
-Dmaven.test.skip=true
fi
- name: Build integrated tests
- if: steps.filter.outputs.k8s-ingress == 'true'
shell: bash
run: |
if mvnd --version > /dev/null 2>&1; then
echo "Using mvnd for build integrated tests"
mvnd -B clean install -Pit -DskipTests -am -f
./shenyu-integrated-test/pom.xml
else
echo "Falling back to maven wrapper for integrated tests"
- if [[ "${{ runner.os }}" == "Windows" ]]; then
- ./mvnw.cmd -B clean install -Pit -DskipTests -am -f
./shenyu-integrated-test/pom.xml
- else
- ./mvnw -B clean install -Pit -DskipTests -am -f
./shenyu-integrated-test/pom.xml
- fi
+ ./mvnw -B clean install -Pit -DskipTests -am -f
./shenyu-integrated-test/pom.xml
fi
Review Comment:
The follow-up `Build integrated tests` step also uses `clean install` (with
`-am`), which will clean and rebuild upstream modules again even though they
were already built earlier in this job. Dropping `clean` (or combining builds)
would avoid extra work and speed up `prepare-images`.
##########
.github/workflows/e2e-k8s.yml:
##########
@@ -261,7 +254,6 @@ jobs:
script: e2e-logging-rocketmq-compose
# - case: shenyu-e2e-case-logging-kafka
# script: e2e-logging-kafka-compose
-
steps:
- uses: actions/checkout@v2
Review Comment:
This job still uses `actions/checkout@v2` while other jobs in this workflow
use `@v3`. `checkout@v2` is deprecated (Node 12-based) and may start failing as
GitHub retires older runtimes; update this to match the newer version used
elsewhere (e.g., `actions/checkout@v3` or `@v4`).
```suggestion
- uses: actions/checkout@v3
```
##########
.github/workflows/e2e-k8s.yml:
##########
@@ -112,48 +109,66 @@ jobs:
mvnd -B clean install -Prelease,docker -Dmaven.javadoc.skip=true
-B -Drat.skip=true -Dmaven.test.skip=true -Djacoco.skip=true -DskipITs
-DskipTests package -T1C
else
echo "Falling back to maven wrapper"
- if [[ "${{ runner.os }}" == "Windows" ]]; then
- ./mvnw.cmd -B clean install -Prelease,docker
-Dmaven.javadoc.skip=true -B -Drat.skip=true -Dmaven.test.skip=true
-Djacoco.skip=true -DskipITs -DskipTests package -T1C
- else
- ./mvnw -B clean install -Prelease,docker
-Dmaven.javadoc.skip=true -B -Drat.skip=true -Dmaven.test.skip=true
-Djacoco.skip=true -DskipITs -DskipTests package -T1C
- fi
+ ./mvnw -B clean install -Prelease,docker -Dmaven.javadoc.skip=true
-B -Drat.skip=true -Dmaven.test.skip=true -Djacoco.skip=true -DskipITs
-DskipTests package -T1C
+ fi
+
+ - name: Build examples with Maven
+ shell: bash
+ run: |
+ if mvnd --version > /dev/null 2>&1; then
+ echo "Using mvnd for build examples"
+ mvnd -B clean install -Pexample -Dmaven.javadoc.skip=true
-Dmaven.test.skip=true -am -f ./shenyu-examples/pom.xml
+ else
+ echo "Falling back to maven wrapper for examples"
+ ./mvnw -B clean install -Pexample -Dmaven.javadoc.skip=true
-Dmaven.test.skip=true -am -f ./shenyu-examples/pom.xml
Review Comment:
`Build examples with Maven` uses `clean install` after the main
`-Prelease,docker` build, which will unnecessarily clean/rebuild any `-am`
dependencies and can significantly increase CI time. Consider removing `clean`
here so this step can reuse artifacts from the previous build (or combine the
builds if feasible).
```suggestion
mvnd -B install -Pexample -Dmaven.javadoc.skip=true
-Dmaven.test.skip=true -am -f ./shenyu-examples/pom.xml
else
echo "Falling back to maven wrapper for examples"
./mvnw -B install -Pexample -Dmaven.javadoc.skip=true
-Dmaven.test.skip=true -am -f ./shenyu-examples/pom.xml
```
--
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]
