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

ci(docs): add a CI step to check for formatting of the documentation #1874

Merged
merged 7 commits into from
Jul 2, 2024

Conversation

SeanCassiere
Copy link
Member

@SeanCassiere SeanCassiere commented Jul 2, 2024

We have a unique problem here, similar to that spider-man meme, where neither of our CI workflows check if there are formatting errors in commits that have documentation-only changes.

The CI workflow (pr.yml) which watches all PRs made (and runs the pnpm test:format script), does not get triggered when only the docs have been modified.
Additionally, the workflow that runs on merges into main (ci.yml) does not actually run the prettier formatting-check script.

This creates a problem where documentation-only PRs can quite easily "look" good and get merged in, without anyone knowing that there is a formatting issue. Later on, it then blocks someone else's code-related PR in the future because the pnpm test:format would fail for them when pr.yml is run on previously unformatted docs.

This CI step would be executed both when a PR is made against and merged into main, checking that prettier passes. I've not included NX Cloud in this runner, since we wouldn't be able to reap any of its benefits, since the prettier script isn't parallelizable.

I've also ignored the docs paths on the existing ci.yml runner since it'd just spin up NX Cloud without actually checking anything meaningful for documentation-only merges.

@SeanCassiere SeanCassiere marked this pull request as ready for review July 2, 2024 01:04
Copy link

nx-cloud bot commented Jul 2, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit b3a58f9. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution


✅ Successfully ran 4 targets

Sent with 💌 from NxCloud.

@lachlancollins
Copy link
Member

@SeanCassiere another option would be to run test:format on CI as well as on PR - this is done by the other TanStack projects (e.g. query, form, table).

I guess if a change is docs only, it would probably be okay to skip CI checks altogether, even if there are formatting issues.

@SeanCassiere
Copy link
Member Author

SeanCassiere commented Jul 2, 2024

I guess if a change is docs only, it would probably be okay to skip CI checks altogether, even if there are formatting issues.

What would you recommend?

The alternative to this would be to:

  • add the test:format script into test:ci so the CI workflow runs on pushes/merges into main, AND
  • remove the path-ignores from the PR workflow.

I'm cool with just maintaining the current two workflows, and making the changes above (to bring it inline with the other projects).

Wanted to get your opinion on this, since the alternative to this approach (listed changes above) requires us to run full CI checks (with the heavy tasks like linting and unit testing) on PRs that have docs-only changes.

@lachlancollins

@lachlancollins
Copy link
Member

  • add the test:format script into test:ci so the CI workflow runs on pushes/merges into main, AND
  • remove the path-ignores from the PR workflow.

@SeanCassiere I agree that test:format should be added to test:ci, but I don't think path-ignores needs to be removed - if it was instead added to ci.yml, no tests will run for docs-only changes. But either way, adding or removing will have the same ultimate outcome which is ensuring both workflows behave more similarly.

@SeanCassiere
Copy link
Member Author

SeanCassiere commented Jul 2, 2024

... but I don't think path-ignores needs to be removed.

Sure thing 👍🏼

🤔 My only concern is that it opens us up to a scenario where I could potentially merge incorrectly formatted docs-only changes, which won't trigger the PR workflow (since paths are ignored), but then after merging the CI workflow would throw an error.
So, a second PR would then need to be opened, where format is run locally and then pushed up in a fresh PR to resolve the errors on main.

@lachlancollins

@lachlancollins
Copy link
Member

🤔 My only concern is that it opens us up to a scenario where I could potentially merge incorrectly formatted docs-only changes, which won't trigger the PR workflow (since paths are ignored), but then after merging the CI workflow would throw an error. So, a second PR would then need to be opened, where format is run locally and then pushed up in a fresh PR to resolve the errors on main.

@SeanCassiere I've just pushed a commit to show what I mean - with this new setup, docs-only changes would be ignored on both pr.yml and ci.yml. The only issue is that the next PR that affects non-docs would need to format the docs. The alternative would be removing paths-ignore from both files, which would be a bit slower but more correct.

@SeanCassiere
Copy link
Member Author

SeanCassiere commented Jul 2, 2024

@lachlancollins I think I prefer to remove path-ignores from both tbh 🤔

Better to block the PR early than have to realize a couple of days later that you need to open a separate PR to fix formatting issues on `main before you can merge 😅.

Thank you!

@SeanCassiere SeanCassiere merged commit e899ca1 into main Jul 2, 2024
9 checks passed
@SeanCassiere SeanCassiere deleted the ci-for-docs branch July 2, 2024 23:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants