Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

build: improve consistency between workflows #41791

Merged
merged 1 commit into from Feb 7, 2022
Merged

build: improve consistency between workflows #41791

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/authors.yml
View file
Open in desktop
@@ -1,4 +1,4 @@
name: "authors update"
name: Authors update
on:
schedule:
# Run once a week at 00:05 AM UTC on Sunday.
Expand Down
12 changes: 6 additions & 6 deletions .github/workflows/auto-start-ci.yml
View file
Open in desktop
Expand Up @@ -12,7 +12,7 @@ env:
NODE_VERSION: lts/*

jobs:
get_prs_for_ci:
get-prs-for-ci:
if: github.repository == 'nodejs/node'
runs-on: ubuntu-latest
outputs:
Expand All @@ -29,20 +29,20 @@ jobs:
--limit 100
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
startCI:
needs: get_prs_for_ci
if: needs.get_prs_for_ci.outputs.numbers != ''
start-ci:
needs: get-prs-for-ci
if: needs.get-prs-for-ci.outputs.numbers != ''
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v2
with:
persist-credentials: false

# Install dependencies
- name: Install Node.js
uses: actions/setup-node@v2
with:
node-version: ${{ env.NODE_VERSION }}

- name: Install node-core-utils
run: npm install -g node-core-utils

Expand All @@ -55,6 +55,6 @@ jobs:
ncu-config set repo "$(echo ${{ github.repository }} | cut -d/ -f2)"
- name: Start the CI
run: ./tools/actions/start-ci.sh ${{ needs.get_prs_for_ci.outputs.numbers }}
run: ./tools/actions/start-ci.sh ${{ needs.get-prs-for-ci.outputs.numbers }}
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
5 changes: 1 addition & 4 deletions .github/workflows/build-tarball.yml
View file
Open in desktop
Expand Up @@ -25,13 +25,12 @@ on:
- '!.github/workflows/build-tarball.yml'

env:
PYTHON_VERSION: '3.10'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need the quotes here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, but I leave them for the other PR #41756.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we do. Otherwise it's parsed as the number 3.1

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should require quotes everywhere after all, YAML is not parsable by humans 😬

FLAKY_TESTS: dontcare

jobs:
build-tarball:
if: github.event.pull_request.draft == false
env:
PYTHON_VERSION: '3.10'
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v2
Expand All @@ -57,8 +56,6 @@ jobs:
name: tarballs
path: tarballs
test-tarball-linux:
env:
PYTHON_VERSION: '3.10'
needs: build-tarball
runs-on: ubuntu-latest
steps:
Expand Down
6 changes: 3 additions & 3 deletions .github/workflows/build-windows.yml
View file
Open in desktop
@@ -1,9 +1,9 @@
name: build-windows
name: Build Windows

on:
pull_request:
paths-ignore:
- "README.md"
- README.md
- .github/**
- '!.github/workflows/build-windows.yml'
types: [opened, synchronize, reopened, ready_for_review]
Expand All @@ -15,7 +15,7 @@ on:
- v[0-9]+.x-staging
- v[0-9]+.x
paths-ignore:
- "README.md"
- README.md
- .github/**
- '!.github/workflows/build-windows.yml'

Expand Down
8 changes: 4 additions & 4 deletions .github/workflows/comment-labeled.yml
View file
Open in desktop
@@ -1,4 +1,4 @@
name: Comment on issues and PRs when labelled
name: Comment on issues and PRs when labeled
on:
issues:
types: [labeled]
Expand All @@ -12,7 +12,7 @@ env:
FAST_TRACK_MESSAGE: Fast-track has been requested by @${{ github.actor }}. Please 👍 to approve.

jobs:
staleComment:
stale-comment:
if: github.repository == 'nodejs/node' && github.event.label.name == 'stalled'
runs-on: ubuntu-latest
steps:
Expand All @@ -22,8 +22,8 @@ jobs:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
run: gh issue comment "$NUMBER" --repo ${{ github.repository }} --body "$STALE_MESSAGE"

fastTrack:
if: github.repository == 'nodejs/node' && github.event_name == 'pull_request_target' && github.event.label.name == 'fast-track'
fast-track:
if: github.repository == 'nodejs/node' && github.event.issue.pull_request && github.event.label.name == 'fast-track'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really understand this change, github.event_name == 'pull_request_target' is not equivalent to !!github.event.issue.pull_request, is it?

Copy link
Member Author

@Mesteery Mesteery Jan 31, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

github.event.issue.pull_request is true when the "issue" is a pull request, so yes it is.
There is an example here: https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#issue_comment-on-issues-only-or-pull-requests-only.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This unfortunately broke the fast-track label flow. See #41888 and https://github.com/nodejs/node/actions/runs/1812810392 where it's skipped.

runs-on: ubuntu-latest
steps:
- name: Request Fast-Track
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/commit-lint.yml
View file
Open in desktop
@@ -1,4 +1,4 @@
name: "First commit message adheres to guidelines at https://goo.gl/p2fr5Q"
name: First commit message adheres to guidelines at https://goo.gl/p2fr5Q
Mesteery marked this conversation as resolved.
Show resolved Hide resolved

on: [pull_request]

Expand Down
6 changes: 2 additions & 4 deletions .github/workflows/coverage-linux.yml
View file
Open in desktop
@@ -1,4 +1,4 @@
name: coverage-linux
name: Coverage Linux

on:
pull_request:
Expand All @@ -11,9 +11,7 @@ on:
- .github/**
- '!.github/workflows/coverage-linux.yml'
push:
branches:
- master
- main
branches: [master, main]
paths-ignore:
- '**.md'
- 'benchmark/**'
Expand Down
6 changes: 2 additions & 4 deletions .github/workflows/coverage-windows.yml
View file
Open in desktop
@@ -1,4 +1,4 @@
name: coverage-windows
name: Coverage Windows

on:
pull_request:
Expand All @@ -12,9 +12,7 @@ on:
- .github/**
- '!.github/workflows/coverage-windows.yml'
push:
branches:
- master
- main
branches: [master, main]
paths-ignore:
- '**.md'
- 'benchmark/**'
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/misc.yml → .github/workflows/doc.yml
View file
Open in desktop
@@ -1,4 +1,4 @@
name: misc
name: Test and upload documentation to artifacts

on:
pull_request:
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/license-builder.yml
View file
Open in desktop
@@ -1,4 +1,4 @@
name: license update
name: License update
on:
schedule:
# 00:00:00 every Monday
Expand Down
4 changes: 2 additions & 2 deletions .github/workflows/linters.yml
View file
Open in desktop
@@ -1,4 +1,4 @@
name: linters
name: Linters

on:
pull_request:
Expand Down Expand Up @@ -121,7 +121,7 @@ jobs:
lint-sh:
if: github.event.pull_request.draft == false
runs-on: ubuntu-20.04
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v2
with:
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/notify-force-push.yml
View file
Open in desktop
Expand Up @@ -8,7 +8,7 @@ name: Notify on Force Push
jobs:
slackNotification:
name: Slack Notification
if: ${{ github.event.forced && github.repository == 'nodejs/node' }}
if: github.repository == 'nodejs/node' && github.event.forced
runs-on: ubuntu-latest
steps:
- name: Slack Notification
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/test-asan.yml
View file
Open in desktop
@@ -1,4 +1,4 @@
name: test-asan
name: Test ASan

on:
pull_request:
Expand Down
8 changes: 3 additions & 5 deletions .github/workflows/test-internet.yml
View file
Open in desktop
@@ -1,4 +1,4 @@
name: test-internet
name: Test internet

on:
workflow_dispatch:
Expand All @@ -7,17 +7,15 @@ on:

pull_request:
types: [opened, synchronize, reopened, ready_for_review]
paths:
- test/internet/**
paths: [test/internet/**]
Copy link
Contributor

@aduh95 aduh95 Jan 31, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this improve the consistency? Are we following a rule or use an auto-formatting tool to decide what syntax to use when writing a list?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed in the different files that when the list is short, this syntax is used.
Unfortunately, Yamllint does not have a rule to enforce this choice.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not blocking comment: I don't think we should use this format of list here, if we add more paths in the future, the other syntax will be more convceniant.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we add more paths in the future and the list becomes more difficult to read or a bit long, we can always use the other syntax.

push:
branches:
- master
- main
- canary
- v[0-9]+.x-staging
- v[0-9]+.x
paths:
- test/internet/**
paths: [test/internet/**]

env:
PYTHON_VERSION: '3.10'
Expand Down
6 changes: 3 additions & 3 deletions .github/workflows/test-linux.yml
View file
Open in desktop
@@ -1,9 +1,9 @@
name: test-linux
name: Test Linux

on:
pull_request:
paths-ignore:
- "README.md"
- README.md
- .github/**
- '!.github/workflows/test-linux.yml'
types: [opened, synchronize, reopened, ready_for_review]
Expand All @@ -15,7 +15,7 @@ on:
- v[0-9]+.x-staging
- v[0-9]+.x
paths-ignore:
- "README.md"
- README.md
- .github/**
- '!.github/workflows/test-linux.yml'

Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/test-macos.yml
View file
Open in desktop
@@ -1,4 +1,4 @@
name: test-macOS
name: Test macOS

on:
pull_request:
Expand Down
4 changes: 2 additions & 2 deletions .github/workflows/tools.yml
View file
Open in desktop
@@ -1,4 +1,4 @@
name: "tools update"
name: Tools update
on:
schedule:
# Run once a week at 00:05 AM UTC on Saturday.
Expand All @@ -7,7 +7,7 @@ on:
workflow_dispatch:

jobs:
tools_update:
tools-update:
if: github.repository == 'nodejs/node'
runs-on: ubuntu-latest
strategy:
Expand Down