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

feat: removes npm/yarn lint from steps #61

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Toolo
Copy link
Contributor

@Toolo Toolo commented Feb 29, 2024

Description

We're running into a situation where parts of yarn lint end up running twice because they are both parth of pre-commit config and the package.json script.

Instead of duplicating, we're going to ensure all pre-commit hooks are executed via pre-commit (or pre-commit run --all-files for general operation).

Changes

  • feat: removes npm/yarn lint from steps

🚀 PR created with fotingo

BREAKING CHANGE: consumers will need to specify required tasks as pre-commit steps, instead of package.json scripts
@Toolo Toolo requested a review from a team as a code owner February 29, 2024 16:02
Copy link

Release notes preview

Below is a preview of the release notes if your PR gets merged.


6.0.0 (2024-02-29)

⚠ BREAKING CHANGES

  • consumers will need to specify required tasks as pre-commit steps, instead of package.json scripts

Features

  • removes npm/yarn lint from steps (6b421e6)

Copy link

Error: missing breaking changes documentation

This pull request contains breaking changes, but no documentation has been added to docs/breaking-changes/v6.md.

Instructions for creating breaking changes document:
mkdir -p docs/breaking-changes
cat <<EOF > docs/breaking-changes/v6.md
# Breaking changes in v6

[//]: # "Brief description of current major version release scope. (remove comment after updating)"

## Description of changes

[//]: # "Elaborate and add context to help the developer understand the changes. (remove comment after updating)"

## Upgrade instructions

[//]: # "Required and suggested prerequisites, example code, etc. (remove comment after updating)"

EOF

Copy link
Contributor

@tagoro9 tagoro9 left a comment

Choose a reason for hiding this comment

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

Just one question + missing breaking changes docs

Comment on lines -36 to -52
- name: Check for yarn.lock
id: check_yarn_lock
uses: andstor/file-existence-action@v3
with:
files: yarn.lock
- name: Install dependencies (yarn)
if: steps.check_yarn_lock.outputs.files_exists == 'true'
shell: bash
run: yarn --pure-lockfile
env:
NPM_AUTH_TOKEN: ${{ inputs.npm-auth-token }}
- name: Install dependencies (npm)
if: steps.check_yarn_lock.outputs.files_exists == 'false'
shell: bash
run: npm ci
env:
NPM_AUTH_TOKEN: ${{ inputs.npm-auth-token }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't we need to still install dependencies so that pre-commit hooks like eslint work?

@armand-sauzay
Copy link
Contributor

armand-sauzay commented Feb 29, 2024

We wanted to do something similar for open-turo/actions-python, looks like the leanest/fastest approach would be to run something along the lines of:

jobs:
  lint:
    name: Lint
    runs-on: [self-hosted, general-ubuntu]
    steps:
      - name: Checkout
        uses: actions/checkout@v4
        with:
          fetch-depth: 0
      - name: Setup python
        uses: actions/setup-python@v5 #or open-turo/action-setup-tools but action-setup-tools is quite slow for new version that are not pre-installed on the runners while the open source ones are caching everything and very fast
      - name: Install pre-commit
        run: pip install pre-commit
      - name: Run pre-commit
        run: pre-commit run --show-diff-on-failure --color=always --all-files

we could also do caching on the pre-commit dependencies but I guess that's another level of optimization. It already runs in 10s-1minute with the above approach (vs 2-4minutes when installing dependencies)

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.

4 participants