You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by GitBox <gi...@apache.org> on 2021/08/17 10:02:35 UTC
[GitHub] [skywalking-banyandb] lujiajing1126 opened a new pull request #42: Separate docker build files
lujiajing1126 opened a new pull request #42:
URL: https://github.com/apache/skywalking-banyandb/pull/42
Signed-off-by: Megrez Lu <lu...@gmail.com>
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] lujiajing1126 commented on a change in pull request #42: Separate docker build files
Posted by GitBox <gi...@apache.org>.
lujiajing1126 commented on a change in pull request #42:
URL: https://github.com/apache/skywalking-banyandb/pull/42#discussion_r690229405
##########
File path: .github/workflows/publish-docker-main.yml
##########
@@ -52,17 +51,13 @@ jobs:
run: make generate
- name: Set up Docker Buildx
uses: docker/setup-buildx-action@v1
- - name: Build docker image
- run: |
- make docker || make docker
- name: Log in to the Container registry
uses: docker/login-action@v1.10.0
- if: github.ref == 'refs/heads/main'
with:
registry: ${{ env.HUB }}
username: ${{ github.actor }}
password: ${{ secrets.GITHUB_TOKEN }}
- - name: Push docker image
- if: github.ref == 'refs/heads/main'
+ - name: Build and Push docker image
run: |
- make docker.push || make docker.push
\ No newline at end of file
+ make docker || make docker
+ make docker.push || make docker.push
Review comment:
> The previous workflow failed to push the Docker images is not because `make docker` and `make docker.push` are in different jobs, it's because `make docker` doesn't even build the Docker images, which is not reproducible in my local machine.
>
> See this https://github.com/apache/skywalking-banyandb/runs/3349244592#step:8:378 , there is no Docker images after `make docker`
There is a warning at the very beginning of the step. The docker image built only exists in the current layer. So we need to build and push in one step.
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] lujiajing1126 commented on a change in pull request #42: Separate docker build files
Posted by GitBox <gi...@apache.org>.
lujiajing1126 commented on a change in pull request #42:
URL: https://github.com/apache/skywalking-banyandb/pull/42#discussion_r690230774
##########
File path: .github/workflows/publish-docker-main.yml
##########
@@ -52,17 +51,13 @@ jobs:
run: make generate
- name: Set up Docker Buildx
uses: docker/setup-buildx-action@v1
- - name: Build docker image
- run: |
- make docker || make docker
- name: Log in to the Container registry
uses: docker/login-action@v1.10.0
- if: github.ref == 'refs/heads/main'
with:
registry: ${{ env.HUB }}
username: ${{ github.actor }}
password: ${{ secrets.GITHUB_TOKEN }}
- - name: Push docker image
- if: github.ref == 'refs/heads/main'
+ - name: Build and Push docker image
run: |
- make docker.push || make docker.push
\ No newline at end of file
+ make docker || make docker
+ make docker.push || make docker.push
Review comment:
https://github.com/apache/skywalking-banyandb/runs/3349154715#step:8:12
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] kezhenxu94 commented on a change in pull request #42: Separate docker build files
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #42:
URL: https://github.com/apache/skywalking-banyandb/pull/42#discussion_r690230903
##########
File path: .github/workflows/publish-docker-main.yml
##########
@@ -52,17 +51,13 @@ jobs:
run: make generate
- name: Set up Docker Buildx
uses: docker/setup-buildx-action@v1
- - name: Build docker image
- run: |
- make docker || make docker
- name: Log in to the Container registry
uses: docker/login-action@v1.10.0
- if: github.ref == 'refs/heads/main'
with:
registry: ${{ env.HUB }}
username: ${{ github.actor }}
password: ${{ secrets.GITHUB_TOKEN }}
- - name: Push docker image
- if: github.ref == 'refs/heads/main'
+ - name: Build and Push docker image
run: |
- make docker.push || make docker.push
\ No newline at end of file
+ make docker || make docker
+ make docker.push || make docker.push
Review comment:
@lujiajing1126 do you mean this `fatal: No names found, cannot describe anything.
30`? It's a git command to set a variable from what I understand and should not affect the docker task.
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] kezhenxu94 merged pull request #42: Separate docker build files
Posted by GitBox <gi...@apache.org>.
kezhenxu94 merged pull request #42:
URL: https://github.com/apache/skywalking-banyandb/pull/42
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] kezhenxu94 commented on a change in pull request #42: Separate docker build files
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #42:
URL: https://github.com/apache/skywalking-banyandb/pull/42#discussion_r690234994
##########
File path: .github/workflows/publish-docker-main.yml
##########
@@ -52,17 +51,13 @@ jobs:
run: make generate
- name: Set up Docker Buildx
uses: docker/setup-buildx-action@v1
- - name: Build docker image
- run: |
- make docker || make docker
- name: Log in to the Container registry
uses: docker/login-action@v1.10.0
- if: github.ref == 'refs/heads/main'
with:
registry: ${{ env.HUB }}
username: ${{ github.actor }}
password: ${{ secrets.GITHUB_TOKEN }}
- - name: Push docker image
- if: github.ref == 'refs/heads/main'
+ - name: Build and Push docker image
run: |
- make docker.push || make docker.push
\ No newline at end of file
+ make docker || make docker
+ make docker.push || make docker.push
Review comment:
@lujiajing1126 check this https://github.com/apache/skywalking-banyandb/runs/3349370271 , I made your changes in this PR run in [my branch](https://github.com/apache/skywalking-banyandb/tree/test), it doesn't help at all
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] lujiajing1126 commented on a change in pull request #42: Separate docker build files
Posted by GitBox <gi...@apache.org>.
lujiajing1126 commented on a change in pull request #42:
URL: https://github.com/apache/skywalking-banyandb/pull/42#discussion_r690243572
##########
File path: .github/workflows/publish-docker-main.yml
##########
@@ -52,17 +51,13 @@ jobs:
run: make generate
- name: Set up Docker Buildx
uses: docker/setup-buildx-action@v1
- - name: Build docker image
- run: |
- make docker || make docker
- name: Log in to the Container registry
uses: docker/login-action@v1.10.0
- if: github.ref == 'refs/heads/main'
with:
registry: ${{ env.HUB }}
username: ${{ github.actor }}
password: ${{ secrets.GITHUB_TOKEN }}
- - name: Push docker image
- if: github.ref == 'refs/heads/main'
+ - name: Build and Push docker image
run: |
- make docker.push || make docker.push
\ No newline at end of file
+ make docker || make docker
+ make docker.push || make docker.push
Review comment:
I've added `--load` options in Makefile.
Pls check the latest CI log. It seems fine now.
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] wu-sheng commented on pull request #42: Separate docker build files
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #42:
URL: https://github.com/apache/skywalking-banyandb/pull/42#issuecomment-900174661
I don't think separating is the key point. GHA shares everything in one CI task.
Let's find what it causes.
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] kezhenxu94 commented on a change in pull request #42: Separate docker build files
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #42:
URL: https://github.com/apache/skywalking-banyandb/pull/42#discussion_r690225452
##########
File path: .github/workflows/publish-docker-pr.yml
##########
@@ -0,0 +1,54 @@
+# 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: publish-docker
+
+on:
+ pull_request:
+
+env:
+ HUB: ghcr.io/apache/skywalking-banyandb
+
+jobs:
+ build:
+ if: github.repository == 'apache/skywalking-banyandb'
+ runs-on: ubuntu-latest
+ permissions:
+ contents: read
+ packages: write
+ timeout-minutes: 90
+ strategy:
+ fail-fast: true
+ env:
+ TAG: ${{ github.sha }}
+ steps:
+ - name: Install Go
+ uses: actions/setup-go@v2
+ with:
+ go-version: 1.16
+ - name: Install goimports
+ run: go install golang.org/x/tools/cmd/goimports@latest
+ - name: Check out code into the Go module directory
+ uses: actions/checkout@v2
+ - name: Update dependencies
+ run: GOPROXY=https://proxy.golang.org go mod download
+ - name: Generate codes
+ run: make generate
+ - name: Set up Docker Buildx
+ uses: docker/setup-buildx-action@v1
Review comment:
I'm very confused why we need so many steps out of Makefile, instead of making them automatically run in Makefile by using dependent tasks so that developers only need to run `make docker` without caring about the GitHub workflow file. The same concern exists in the other workflow file `.github/workflows/publish-docker-main.yml`
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] kezhenxu94 commented on a change in pull request #42: Separate docker build files
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #42:
URL: https://github.com/apache/skywalking-banyandb/pull/42#discussion_r690240912
##########
File path: .github/workflows/publish-docker-main.yml
##########
@@ -52,17 +51,13 @@ jobs:
run: make generate
- name: Set up Docker Buildx
uses: docker/setup-buildx-action@v1
- - name: Build docker image
- run: |
- make docker || make docker
- name: Log in to the Container registry
uses: docker/login-action@v1.10.0
- if: github.ref == 'refs/heads/main'
with:
registry: ${{ env.HUB }}
username: ${{ github.actor }}
password: ${{ secrets.GITHUB_TOKEN }}
- - name: Push docker image
- if: github.ref == 'refs/heads/main'
+ - name: Build and Push docker image
run: |
- make docker.push || make docker.push
\ No newline at end of file
+ make docker || make docker
+ make docker.push || make docker.push
Review comment:
> Also, separating the `publish-docker` workflow into 2 files makes no sense IMO, the reason why we want the workflow to run (partially though) in PRs is that we want to check the commands in that workflow is runnable and don't want it to break after merging. Separating it into 2 files may lead to a situation where developers adjust the file `.github/workflows/publish-docker-pr.yml` to make the CI pass and merge the PRs, but forget to adjust `.github/workflows/publish-docker-main.yml`.
What I suggested is something like this:
```suggestion
make docker || make docker
if [[ ${{ github.ref == 'refs/heads/main' ]]; then
make docker.push || make docker.push
fi
```
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] lujiajing1126 commented on a change in pull request #42: Separate docker build files
Posted by GitBox <gi...@apache.org>.
lujiajing1126 commented on a change in pull request #42:
URL: https://github.com/apache/skywalking-banyandb/pull/42#discussion_r690231193
##########
File path: .github/workflows/publish-docker-main.yml
##########
@@ -52,17 +51,13 @@ jobs:
run: make generate
- name: Set up Docker Buildx
uses: docker/setup-buildx-action@v1
- - name: Build docker image
- run: |
- make docker || make docker
- name: Log in to the Container registry
uses: docker/login-action@v1.10.0
- if: github.ref == 'refs/heads/main'
with:
registry: ${{ env.HUB }}
username: ${{ github.actor }}
password: ${{ secrets.GITHUB_TOKEN }}
- - name: Push docker image
- if: github.ref == 'refs/heads/main'
+ - name: Build and Push docker image
run: |
- make docker.push || make docker.push
\ No newline at end of file
+ make docker || make docker
+ make docker.push || make docker.push
Review comment:
@kezhenxu94 I mean the following line. The link may be misleading...
```
time="2021-08-17T09:53:39Z" level=warning msg="No output specified for docker-container driver. Build result will only remain in the build cache. To push result image into registry use --push or to load image into docker use --load"
```
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] kezhenxu94 commented on a change in pull request #42: Separate docker build files
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #42:
URL: https://github.com/apache/skywalking-banyandb/pull/42#discussion_r690235932
##########
File path: .github/workflows/publish-docker-pr.yml
##########
@@ -0,0 +1,54 @@
+# 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: publish-docker
+
+on:
+ pull_request:
+
+env:
+ HUB: ghcr.io/apache/skywalking-banyandb
+
+jobs:
+ build:
+ if: github.repository == 'apache/skywalking-banyandb'
+ runs-on: ubuntu-latest
+ permissions:
+ contents: read
+ packages: write
+ timeout-minutes: 90
+ strategy:
+ fail-fast: true
+ env:
+ TAG: ${{ github.sha }}
+ steps:
+ - name: Install Go
+ uses: actions/setup-go@v2
+ with:
+ go-version: 1.16
+ - name: Install goimports
+ run: go install golang.org/x/tools/cmd/goimports@latest
+ - name: Check out code into the Go module directory
+ uses: actions/checkout@v2
+ - name: Update dependencies
+ run: GOPROXY=https://proxy.golang.org go mod download
+ - name: Generate codes
+ run: make generate
+ - name: Set up Docker Buildx
+ uses: docker/setup-buildx-action@v1
Review comment:
> I suppose we can move `goimport` installation into Makefile and have a better dependency tree
Not only `goimport`, but also `make generate`
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] lujiajing1126 commented on pull request #42: Separate docker build files
Posted by GitBox <gi...@apache.org>.
lujiajing1126 commented on pull request #42:
URL: https://github.com/apache/skywalking-banyandb/pull/42#issuecomment-900183760
> Also, separating the `publish-docker` workflow into 2 files makes no sense IMO, the reason why we want the workflow to run (partially though) in PRs is that we want to check the commands in that workflow is runnable and don't want it to break after merging. Separating it into 2 files may lead to a situation where developers adjust the file `.github/workflows/publish-docker-pr.yml` to make the CI pass and merge the PRs, but forget to adjust `.github/workflows/publish-docker-main.yml`.
This change has been reverted.
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] lujiajing1126 commented on a change in pull request #42: Separate docker build files
Posted by GitBox <gi...@apache.org>.
lujiajing1126 commented on a change in pull request #42:
URL: https://github.com/apache/skywalking-banyandb/pull/42#discussion_r690234148
##########
File path: .github/workflows/publish-docker-pr.yml
##########
@@ -0,0 +1,54 @@
+# 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: publish-docker
+
+on:
+ pull_request:
+
+env:
+ HUB: ghcr.io/apache/skywalking-banyandb
+
+jobs:
+ build:
+ if: github.repository == 'apache/skywalking-banyandb'
+ runs-on: ubuntu-latest
+ permissions:
+ contents: read
+ packages: write
+ timeout-minutes: 90
+ strategy:
+ fail-fast: true
+ env:
+ TAG: ${{ github.sha }}
+ steps:
+ - name: Install Go
+ uses: actions/setup-go@v2
+ with:
+ go-version: 1.16
+ - name: Install goimports
+ run: go install golang.org/x/tools/cmd/goimports@latest
+ - name: Check out code into the Go module directory
+ uses: actions/checkout@v2
+ - name: Update dependencies
+ run: GOPROXY=https://proxy.golang.org go mod download
+ - name: Generate codes
+ run: make generate
+ - name: Set up Docker Buildx
+ uses: docker/setup-buildx-action@v1
Review comment:
I suppose we can move `goimport` installation into Makefile and have a better dependency tree
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] lujiajing1126 commented on a change in pull request #42: Separate docker build files
Posted by GitBox <gi...@apache.org>.
lujiajing1126 commented on a change in pull request #42:
URL: https://github.com/apache/skywalking-banyandb/pull/42#discussion_r690231193
##########
File path: .github/workflows/publish-docker-main.yml
##########
@@ -52,17 +51,13 @@ jobs:
run: make generate
- name: Set up Docker Buildx
uses: docker/setup-buildx-action@v1
- - name: Build docker image
- run: |
- make docker || make docker
- name: Log in to the Container registry
uses: docker/login-action@v1.10.0
- if: github.ref == 'refs/heads/main'
with:
registry: ${{ env.HUB }}
username: ${{ github.actor }}
password: ${{ secrets.GITHUB_TOKEN }}
- - name: Push docker image
- if: github.ref == 'refs/heads/main'
+ - name: Build and Push docker image
run: |
- make docker.push || make docker.push
\ No newline at end of file
+ make docker || make docker
+ make docker.push || make docker.push
Review comment:
```
time="2021-08-17T09:53:39Z" level=warning msg="No output specified for docker-container driver. Build result will only remain in the build cache. To push result image into registry use --push or to load image into docker use --load"
```
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] kezhenxu94 commented on a change in pull request #42: Separate docker build files
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #42:
URL: https://github.com/apache/skywalking-banyandb/pull/42#discussion_r690227859
##########
File path: .github/workflows/publish-docker-main.yml
##########
@@ -52,17 +51,13 @@ jobs:
run: make generate
- name: Set up Docker Buildx
uses: docker/setup-buildx-action@v1
- - name: Build docker image
- run: |
- make docker || make docker
- name: Log in to the Container registry
uses: docker/login-action@v1.10.0
- if: github.ref == 'refs/heads/main'
with:
registry: ${{ env.HUB }}
username: ${{ github.actor }}
password: ${{ secrets.GITHUB_TOKEN }}
- - name: Push docker image
- if: github.ref == 'refs/heads/main'
+ - name: Build and Push docker image
run: |
- make docker.push || make docker.push
\ No newline at end of file
+ make docker || make docker
+ make docker.push || make docker.push
Review comment:
The previous workflow failed to push the Docker images is not because `make docker` and `make docker.push` are in different jobs, it's because `make docker` doesn't even build the Docker images, which is not reproducible in my local machine.
See this https://github.com/apache/skywalking-banyandb/runs/3349244592#step:8:378 , there is no Docker images after `make 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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org