You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@pekko.apache.org by "mdedetrich (via GitHub)" <gi...@apache.org> on 2023/02/21 11:47:09 UTC
[GitHub] [incubator-pekko-http] mdedetrich opened a new pull request, #77: Move header check into its own github actions workflow
mdedetrich opened a new pull request, #77:
URL: https://github.com/apache/incubator-pekko-http/pull/77
This PR adds a header check as its own addition github actions workflow and moves it away from `validatePullRequest`. The context behind this change is that at some point we want to add a required status check on PR's to make sure that the headers are correct and due to how brittle nature of status checks (more specifically, its possible to block a repo if you get the name of a status check wrong which then involves making an INFRA ticket to unblock it) hence it makes sense to have these checks as their own separate workflows with a name that is highly unlikely to change (in this case `Check headers`).
The current implementation is mainly copied from https://github.com/apache/incubator-pekko/blob/main/.github/workflows/build-test-prValidation.yml#L1-L41. One notable difference from the implementation in pekko core is the `name` field was changed from `Check / Headers` to `Check headers` . This is important because its that `name` field which determines the github status check, so the reasoning behind the change to `Check headers` is that its consistent with the already existing `Code is formatted` status check (see https://github.com/apache/incubator-pekko-http/blob/main/.asf.yaml#L37). The presumption is that if this PR is merged then a future one will be done in pekko core to make the `Check headers` consistent amongst all of our pekko repos.
--
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@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] mdedetrich commented on a diff in pull request #77: Move header check into its own github actions workflow
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #77:
URL: https://github.com/apache/incubator-pekko-http/pull/77#discussion_r1112973500
##########
.github/workflows/check-headers.yml:
##########
@@ -0,0 +1,32 @@
+name: Headers
+
+on:
+ pull_request:
+
+permissions: {}
+
+jobs:
+ check-headers:
+ name: Check headers
+ runs-on: ubuntu-20.04
+ steps:
+ - name: Checkout
+ uses: actions/checkout@v3
+ with:
+ # we don't know what commit the last tag was it's safer to get entire repo so previousStableVersion resolves
Review Comment:
Is this still necessary given that the header check is now moved outside of `validatePullRequest` and being done automatically on each PR? This was copied since it was also in pekko-core.
--
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@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] pjfanning commented on a diff in pull request #77: Move header check into its own github actions workflow
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #77:
URL: https://github.com/apache/incubator-pekko-http/pull/77#discussion_r1112996563
##########
.github/workflows/check-headers.yml:
##########
@@ -0,0 +1,32 @@
+name: Headers
+
+on:
+ pull_request:
+
+permissions: {}
+
+jobs:
+ check-headers:
+ name: Check headers
+ runs-on: ubuntu-20.04
+ steps:
+ - name: Checkout
+ uses: actions/checkout@v3
+ with:
+ # we don't know what commit the last tag was it's safer to get entire repo so previousStableVersion resolves
+ fetch-depth: 0
+
+ - name: Setup Java 8
+ uses: actions/setup-java@v3
+ with:
+ distribution: temurin
+ java-version: 8
+
+ - name: Cache Coursier cache
+ uses: coursier/cache-action@v6.4.0
+
+ - name: Enable jvm-opts
+ run: cp .jvmopts-ghactions .jvmopts
+
+ - name: Check headers
+ run: sbt +headerCheckAll
Review Comment:
I've found that `headerCreate` some times need a `clean`, that it seems to ignore files that have not been worked on recently.
--
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@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] jrudolph commented on a diff in pull request #77: Move header check into its own github actions workflow
Posted by "jrudolph (via GitHub)" <gi...@apache.org>.
jrudolph commented on code in PR #77:
URL: https://github.com/apache/incubator-pekko-http/pull/77#discussion_r1114036970
##########
.github/workflows/check-headers.yml:
##########
@@ -0,0 +1,32 @@
+name: Headers
+
+on:
+ pull_request:
+
+permissions: {}
+
+jobs:
+ check-headers:
+ name: Check headers
+ runs-on: ubuntu-20.04
+ steps:
+ - name: Checkout
+ uses: actions/checkout@v3
+ with:
+ # we don't know what commit the last tag was it's safer to get entire repo so previousStableVersion resolves
+ fetch-depth: 0
+
+ - name: Setup Java 8
+ uses: actions/setup-java@v3
+ with:
+ distribution: temurin
+ java-version: 8
+
+ - name: Cache Coursier cache
+ uses: coursier/cache-action@v6.4.0
+
+ - name: Enable jvm-opts
+ run: cp .jvmopts-ghactions .jvmopts
+
+ - name: Check headers
+ run: sbt +headerCheckAll
Review Comment:
Great, even better if it works like that.
--
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@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] mdedetrich commented on a diff in pull request #77: Move header check into its own github actions workflow
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #77:
URL: https://github.com/apache/incubator-pekko-http/pull/77#discussion_r1113147964
##########
.github/workflows/check-headers.yml:
##########
@@ -0,0 +1,32 @@
+name: Headers
+
+on:
+ pull_request:
+
+permissions: {}
+
+jobs:
+ check-headers:
+ name: Check headers
+ runs-on: ubuntu-20.04
+ steps:
+ - name: Checkout
+ uses: actions/checkout@v3
+ with:
+ # we don't know what commit the last tag was it's safer to get entire repo so previousStableVersion resolves
+ fetch-depth: 0
+
+ - name: Setup Java 8
+ uses: actions/setup-java@v3
+ with:
+ distribution: temurin
+ java-version: 8
+
+ - name: Cache Coursier cache
+ uses: coursier/cache-action@v6.4.0
+
+ - name: Enable jvm-opts
+ run: cp .jvmopts-ghactions .jvmopts
+
+ - name: Check headers
+ run: sbt +headerCheckAll
Review Comment:
I am going to go ahead and merge this, there is a strong chance the discrepancy is like due to what @pjfanning stated in which case I don't see how it would be an issue for CI (can always improve this if its not the case).
--
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@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] mdedetrich commented on a diff in pull request #77: Move header check into its own github actions workflow
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #77:
URL: https://github.com/apache/incubator-pekko-http/pull/77#discussion_r1112973500
##########
.github/workflows/check-headers.yml:
##########
@@ -0,0 +1,32 @@
+name: Headers
+
+on:
+ pull_request:
+
+permissions: {}
+
+jobs:
+ check-headers:
+ name: Check headers
+ runs-on: ubuntu-20.04
+ steps:
+ - name: Checkout
+ uses: actions/checkout@v3
+ with:
+ # we don't know what commit the last tag was it's safer to get entire repo so previousStableVersion resolves
Review Comment:
Is this still necessary given that the header check is now moved outside of `validatePullRequest` and being done automatically on each PR?
--
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@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] mdedetrich commented on a diff in pull request #77: Move header check into its own github actions workflow
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #77:
URL: https://github.com/apache/incubator-pekko-http/pull/77#discussion_r1112981281
##########
.github/workflows/check-headers.yml:
##########
@@ -0,0 +1,32 @@
+name: Headers
+
+on:
+ pull_request:
+
+permissions: {}
+
+jobs:
+ check-headers:
+ name: Check headers
+ runs-on: ubuntu-20.04
+ steps:
+ - name: Checkout
+ uses: actions/checkout@v3
+ with:
+ # we don't know what commit the last tag was it's safer to get entire repo so previousStableVersion resolves
+ fetch-depth: 0
+
+ - name: Setup Java 8
+ uses: actions/setup-java@v3
+ with:
+ distribution: temurin
+ java-version: 8
+
+ - name: Cache Coursier cache
+ uses: coursier/cache-action@v6.4.0
+
+ - name: Enable jvm-opts
+ run: cp .jvmopts-ghactions .jvmopts
+
+ - name: Check headers
+ run: sbt +headerCheckAll
Review Comment:
So I just tried locally and `sbt +headerCheckAll` also seems to work with multi jvm tests? To do this I manually removed the header from `http-tests/src/multi-jvm/scala/org/apache/pekko/http/PekkoHttpServerLatencyMultiNodeSpec.scala`, ran `sbt +headerCheckAll` and it failed, i.e.
```
<@incubator-pekko-http>-<⎇ create-header-check-workflow-and-remove-from-validatePullRequest>-<*>-> sbt +headerCheckAll
[info] welcome to sbt 1.8.2 (Amazon.com Inc. Java 1.8.0_362)
[info] loading global plugins from /Users/mdedetrich/.sbt/1.0/plugins
[info] loading settings for project incubator-pekko-http-build from plugins.sbt ...
[info] loading project definition from /Users/mdedetrich/github/incubator-pekko-http/project
[info] compiling 1 Scala source to /Users/mdedetrich/github/incubator-pekko-http/project/target/scala-2.12/sbt-1.0/classes ...
[info] loading settings for project pekko-http from build.sbt ...
[info] resolving key references (32390 settings) ...
[info] Building Pekko HTTP 0.0.0+4290-4c0b8efe+20230221-1321-SNAPSHOT against Pekko 0.0.0+26592-864ee821-SNAPSHOT on Scala 2.13.8
[info] set current project to pekko-http-root (in build file:/Users/mdedetrich/github/incubator-pekko-http/)
[info] Setting Scala version to 2.12.15 on 21 projects.
[info] Reapplying settings...
[info] Building Pekko HTTP 0.0.0+4290-4c0b8efe+20230221-1321-SNAPSHOT against Pekko 0.0.0+26592-864ee821-SNAPSHOT on Scala 2.12.15
[info] set current project to pekko-http-root (in build file:/Users/mdedetrich/github/incubator-pekko-http/)
[error] There are files without headers!
[error] /Users/mdedetrich/github/incubator-pekko-http/http-tests/src/multi-jvm/scala/org/apache/pekko/http/PekkoHttpServerLatencyMultiNodeSpec.scala
[error] (http-tests / Multi-jvm / headerCheck) There are files without headers!
[error] /Users/mdedetrich/github/incubator-pekko-http/http-tests/src/multi-jvm/scala/org/apache/pekko/http/PekkoHttpServerLatencyMultiNodeSpec.scala
[error] Total time: 0 s, completed Feb 21, 2023 1:21:56 PM
```
--
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@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] jrudolph commented on a diff in pull request #77: Move header check into its own github actions workflow
Posted by "jrudolph (via GitHub)" <gi...@apache.org>.
jrudolph commented on code in PR #77:
URL: https://github.com/apache/incubator-pekko-http/pull/77#discussion_r1112974946
##########
.github/workflows/check-headers.yml:
##########
@@ -0,0 +1,32 @@
+name: Headers
+
+on:
+ pull_request:
+
+permissions: {}
+
+jobs:
+ check-headers:
+ name: Check headers
+ runs-on: ubuntu-20.04
+ steps:
+ - name: Checkout
+ uses: actions/checkout@v3
+ with:
+ # we don't know what commit the last tag was it's safer to get entire repo so previousStableVersion resolves
+ fetch-depth: 0
+
+ - name: Setup Java 8
+ uses: actions/setup-java@v3
+ with:
+ distribution: temurin
+ java-version: 8
+
+ - name: Cache Coursier cache
+ uses: coursier/cache-action@v6.4.0
+
+ - name: Enable jvm-opts
+ run: cp .jvmopts-ghactions .jvmopts
+
+ - name: Check headers
+ run: sbt +headerCheckAll
Review Comment:
It doesn't aggregate so the project needs to be specified manually:
```suggestion
run: sbt +headerCheckAll "+;http-tests/MultiJvm/headerCheck"
```
##########
.github/workflows/check-headers.yml:
##########
@@ -0,0 +1,32 @@
+name: Headers
+
+on:
+ pull_request:
+
+permissions: {}
+
+jobs:
+ check-headers:
+ name: Check headers
+ runs-on: ubuntu-20.04
+ steps:
+ - name: Checkout
+ uses: actions/checkout@v3
+ with:
+ # we don't know what commit the last tag was it's safer to get entire repo so previousStableVersion resolves
+ fetch-depth: 0
+
+ - name: Setup Java 8
+ uses: actions/setup-java@v3
+ with:
+ distribution: temurin
+ java-version: 8
+
+ - name: Cache Coursier cache
+ uses: coursier/cache-action@v6.4.0
+
+ - name: Enable jvm-opts
+ run: cp .jvmopts-ghactions .jvmopts
+
+ - name: Check headers
+ run: sbt +headerCheckAll
Review Comment:
Probably also needs `http-tests / MultiJvm / headerCheck`.
--
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@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] mdedetrich commented on a diff in pull request #77: Move header check into its own github actions workflow
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #77:
URL: https://github.com/apache/incubator-pekko-http/pull/77#discussion_r1112972450
##########
.github/workflows/check-headers.yml:
##########
@@ -0,0 +1,32 @@
+name: Headers
+
+on:
+ pull_request:
+
+permissions: {}
+
+jobs:
+ check-headers:
+ name: Check headers
+ runs-on: ubuntu-20.04
+ steps:
+ - name: Checkout
+ uses: actions/checkout@v3
+ with:
Review Comment:
Is this still necessary given that the header check is now moved outside of `validatePullRequest` and being done automatically on each PR?
--
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@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] jrudolph commented on a diff in pull request #77: Move header check into its own github actions workflow
Posted by "jrudolph (via GitHub)" <gi...@apache.org>.
jrudolph commented on code in PR #77:
URL: https://github.com/apache/incubator-pekko-http/pull/77#discussion_r1112978942
##########
.github/workflows/check-headers.yml:
##########
@@ -0,0 +1,32 @@
+name: Headers
+
+on:
+ pull_request:
+
+permissions: {}
+
+jobs:
+ check-headers:
+ name: Check headers
+ runs-on: ubuntu-20.04
+ steps:
+ - name: Checkout
+ uses: actions/checkout@v3
+ with:
+ # we don't know what commit the last tag was it's safer to get entire repo so previousStableVersion resolves
Review Comment:
I don't think we need it for the check, good point.
--
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@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] mdedetrich merged pull request #77: Move header check into its own github actions workflow
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich merged PR #77:
URL: https://github.com/apache/incubator-pekko-http/pull/77
--
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@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] mdedetrich commented on a diff in pull request #77: Move header check into its own github actions workflow
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #77:
URL: https://github.com/apache/incubator-pekko-http/pull/77#discussion_r1112998863
##########
.github/workflows/check-headers.yml:
##########
@@ -0,0 +1,32 @@
+name: Headers
+
+on:
+ pull_request:
+
+permissions: {}
+
+jobs:
+ check-headers:
+ name: Check headers
+ runs-on: ubuntu-20.04
+ steps:
+ - name: Checkout
+ uses: actions/checkout@v3
+ with:
+ # we don't know what commit the last tag was it's safer to get entire repo so previousStableVersion resolves
+ fetch-depth: 0
+
+ - name: Setup Java 8
+ uses: actions/setup-java@v3
+ with:
+ distribution: temurin
+ java-version: 8
+
+ - name: Cache Coursier cache
+ uses: coursier/cache-action@v6.4.0
+
+ - name: Enable jvm-opts
+ run: cp .jvmopts-ghactions .jvmopts
+
+ - name: Check headers
+ run: sbt +headerCheckAll
Review Comment:
I can see how this issue can occur locally but is this relevant for CI on a fresh checkout?
--
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@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] mdedetrich commented on a diff in pull request #77: Move header check into its own github actions workflow
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #77:
URL: https://github.com/apache/incubator-pekko-http/pull/77#discussion_r1112989040
##########
.github/workflows/check-headers.yml:
##########
@@ -0,0 +1,32 @@
+name: Headers
+
+on:
+ pull_request:
+
+permissions: {}
+
+jobs:
+ check-headers:
+ name: Check headers
+ runs-on: ubuntu-20.04
+ steps:
+ - name: Checkout
+ uses: actions/checkout@v3
+ with:
+ # we don't know what commit the last tag was it's safer to get entire repo so previousStableVersion resolves
Review Comment:
Cool, I just removed it and force pushed PR
--
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@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org