Skip to content
This repository has been archived by the owner on Nov 18, 2024. It is now read-only.

Add option to make npm install optional #5

Merged
merged 5 commits into from
Jul 3, 2020

Conversation

luisherranz
Copy link
Contributor

First of all, thanks for the action. It's awesome 🙂

We had a problem using it, though. We use a lerna monorepo and the npm install that you do inside the docker container fails, so I've added an option to avoid running npm install in the container and thus allow people to run their necessary install and/or build scripts in a previous step of their workflow, like this:

jobs:
  eslint:
    runs-on: ubuntu-latest
    steps:
      - name: Checkout Repo
        uses: actions/checkout@v2
      - name: Install dependencies
        run: npm ci
      - name: Run a build step
        run: npm run build
      - uses: bradennapier/eslint-plus-action@v2
        with:
          github-token: ${{secrets.GITHUB_TOKEN}}
          npmInstall: false

https://github.com/frontity/frontity/blob/refactor-analytics-event/.github/workflows/eslint.yml

The option is called npmInstall and has a default of true.

Let me know what you think.

@bradennapier
Copy link
Owner

Yeah it makes sense that this is an issue - my goal was to reduce the steps needed to get this working but it makes sense to set this up better - going to see if there may be a more automated way of doing it.

Copy link
Owner

@bradennapier bradennapier left a comment

Choose a reason for hiding this comment

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

Let me know your thoughts on the suggested changes. We should probably also update the readme as-to how this would work.

entrypoint.sh Outdated Show resolved Hide resolved
entrypoint.sh Outdated Show resolved Hide resolved
action.yml Outdated Show resolved Hide resolved
action.yml Outdated Show resolved Hide resolved
luisherranz and others added 3 commits July 3, 2020 11:50
As suggested by @bradennapier.

Co-authored-by: Braden Napier <bradynapier+github@gmail.com>
Co-authored-by: Braden Napier <bradynapier+github@gmail.com>
I've also removed the example because now that this is automatic I don't think it makes sense anymore.
@bradennapier
Copy link
Owner

bradennapier commented Jul 3, 2020

Ok will merge these in, however I am going to see about waiting until this major release I am working on that should make it possible to lint remote forks securely. I will let you know when released (as v3 most likely).

see: #11

@bradennapier bradennapier merged commit a578c5e into bradennapier:master Jul 3, 2020
@luisherranz
Copy link
Contributor Author

No problem. I just subscribed to the repo 🙂

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants