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.

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

chore: Split Docs CI from core CI #17897

Merged
merged 5 commits into from Jan 11, 2024
Merged

chore: Split Docs CI from core CI #17897

merged 5 commits into from Jan 11, 2024

Conversation

nzakas
Copy link
Member

@nzakas nzakas commented Dec 22, 2023

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[x] Other, please explain:

CI update

What changes did you make? (Give an overview)

This is an attempt to split out the docs-related CI from everything else to help non-docs-related PRs finish their CI faster.

I've split out the docs-related CI tasks into a separate workflow that only runs when files are updated in the docs/ directory.

Is there anything you'd like reviewers to focus on?

Did I miss anything?

@nzakas nzakas requested a review from a team as a code owner December 22, 2023 00:20
@eslint-github-bot eslint-github-bot bot added the chore This change is not user-facing label Dec 22, 2023
Copy link

netlify bot commented Dec 22, 2023

Deploy Preview for docs-eslint canceled.

Name Link
🔨 Latest commit a924b50
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/659dcb0a3eacaf000847224b

Copy link
Contributor

@snitin315 snitin315 left a comment

Choose a reason for hiding this comment

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

LGTM. I will leave it open for others to review.

@snitin315 snitin315 added the accepted There is consensus among the team that this change meets the criteria for inclusion label Dec 22, 2023
Copy link
Member

@harish-sethuraman harish-sethuraman left a comment

Choose a reason for hiding this comment

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

LGTM

@harish-sethuraman
Copy link
Member

Only doubt I have is we have added paths for the docs CI. Should we consider excluding the docs in the normal one then?
Any change inside docs directory only might trigger the main CI as well? Just wanted to confirm on this alone

Just in case:

 paths-ignore:
      - 'docs/**'

@mdjermanovic
Copy link
Member

Should we consider excluding the docs in the normal one then?

Some checks verify consistency between code and docs, so either change should trigger them and thus we'd need to split checks into more than 2 sets.

Comment on lines 34 to 37
- name: Lint Docs JS Files
run: node Makefile lintDocsJS
- name: Check Rule Examples
run: node Makefile checkRuleExamples
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure about these two. A change in our eslint config or in the tool that checks rule examples can cause these checks to fail. But those changes are not in docs/** so they wouldn't trigger these checks and so the failures might go unnoticed until someone submits a PR that changes something in the docs.

Maybe only those tasks that run in working-directory: docs can be safely extracted into a separate CI?

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense. 👍

.github/workflows/ci.yml Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/docs-ci.yml Outdated Show resolved Hide resolved
@mdjermanovic
Copy link
Member

There are merge conflicts now.

Comment on lines 3 to 10
push:
branches: [main]
paths:
- 'docs/**'

pull_request:
branches: [main]

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we also add the same paths for pull_request?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ooh good catch!

@nzakas nzakas merged commit 1bf2520 into main Jan 11, 2024
17 checks passed
@nzakas nzakas deleted the docs-ci branch January 11, 2024 19:18
bmish added a commit to bmish/eslint that referenced this pull request Jan 16, 2024
* main:
  chore: Split Docs CI from core CI (eslint#17897)
  fix: Ensure config keys are printed for config errors (eslint#17980)
  chore: delete relative-module-resolver.js (eslint#17981)
  docs: migration guide entry for `no-inner-declarations` (eslint#17977)
  docs: Update README
  feat: maintain latest ecma version in ESLint (eslint#17958)
  feat!: no-inner-declaration new default behaviour and option (eslint#17885)
  feat: add `no-useless-assignment` rule (eslint#17625)
  fix: `no-misleading-character-class` edge cases with granular errors (eslint#17970)
  feat: `no-misleading-character-class` granular errors (eslint#17515)
  docs: fix number of code-path events on custom rules page (eslint#17969)
  docs: reorder entries in v9 migration guide (eslint#17967)
  fix!: handle `--output-file` for empty output when saving to disk (eslint#17957)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion chore This change is not user-facing
Projects
Status: Complete
Development

Successfully merging this pull request may close these issues.

None yet

4 participants