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

node_modules for real? #12

Closed
chenyong opened this issue Jun 1, 2020 · 15 comments · Fixed by #51
Closed

node_modules for real? #12

chenyong opened this issue Jun 1, 2020 · 15 comments · Fixed by #51
Assignees

Comments

@chenyong
Copy link

chenyong commented Jun 1, 2020

I see node_modules/ included in the project while it's considered bad practice to have it in git repo. Is is the only way to make sure Node runs well in actions with correct dependencies? Can I install the packages during(or before) action is really running?

@chenyong chenyong changed the title node_modules for sure? node_modules for real? Jun 1, 2020
@IgnorantSapient
Copy link

IgnorantSapient commented Nov 4, 2020

@chenyong I agree with you 100%

However, here is a workaround if node_modules is in your gitignore and you wish to generate node_modules during runtime:

on: [push]

jobs:
  hello_world_job:
    runs-on: ubuntu-latest
    name: A job to say hello
    steps:
      # To use this repository's private action,
      # you must check out the repository
      - name: Checkout
        uses: actions/checkout@v2
      - name: Install Packages
        run: npm install
      - name: Hello world action step
        uses: ./ # Uses an action in the root directory
        id: hello
        with:
          who-to-greet: "Mona the Octocat!!!!!"
      # Use the output from the `hello` step
      - name: Get the output time
        run: echo "The time was ${{ steps.hello.outputs.time }}"

The above mentioned code is a remix of the code given here

@adamhenson
Copy link

adamhenson commented Nov 4, 2020

@123aswin123 in your example you are consuming an action from a workflow which is not the same as creating an action. This project is an example of how to create a JavaScript GitHub Action. The problem this issue points out is the inability to create an action without including node_modules.

@irl-segfault
Copy link

irl-segfault commented Nov 30, 2021

bump, any way we can just have the actions runner npm install to save us all from checking in node_modules, and alternatively to avoid an extra manual npm install hack step?

@flagbird
Copy link

flagbird commented Dec 9, 2021

Probably it's a bad practice that the direct content of a repository is the action in such a case. One (not so elegant) solution could be to have one repo for the code that you'll work with and another repo where you deploy a build-result to, that's going to be the action you will use.

@adamhenson
Copy link

adamhenson commented Dec 9, 2021

@rachmari, @andymckay, @jorgebg, @thboop - do you have any thoughts about this issue?

@rachmari
Copy link
Collaborator

rachmari commented Dec 9, 2021

Hi @adamhenson 👋. This repo is a companion to the documentation in this guide: https://docs.github.com/en/actions/creating-actions/creating-a-javascript-action#commit-tag-and-push-your-action-to-github

While checking in the node_modules directory is counterintuitive, here is a bit of documentation about why we require you to check in the node_modules directory when creating an action (from the guide 👆):

GitHub downloads each action run in a workflow during runtime and executes it as a complete package of code before you can use workflow commands like run to interact with the runner machine. This means you must include any package dependencies required to run the JavaScript code.

I've reached out to see if there are any plans to change that in the future. I'll drop a note here if I hear about any plans to change that going forward.

@adamhenson
Copy link

Thanks @rachmari. If there is a better place to open this issue - please do let me know where. Appreciate the link to the docs but it’s still unclear why @irl-segfault’s suggestion isn’t possible. This issue is impactful in the open source sense in that PRs are quite difficult to manage with the diff from node_modules or even the Vercel option which generates a gigantic diff (and threw errors for me).

@rachmari
Copy link
Collaborator

rachmari commented Dec 9, 2021

If there is a better place to open this issue - please do let me know where.

@adamhenson finding an existing discussion or opening a new one here is the best place: https://github.com/github/feedback/discussions/categories/actions-and-packages-feedback

Appreciate the link to the docs but it’s still unclear why @irl-segfault’s suggestion isn’t possible.

The actions team doesn't currently have any plans to update the runners to be able to perform the npm install for each JavaScript action defined in a workflow. I suspect that could increase workflow time significantly in some cases as well, depending on how many actions you use in a single workflow.

Rather than checking in the entire directory of node_modules, another option that the guide refers to is using https://github.com/vercel/ncc to compile all of the node modules into a single file. I'm not sure if that's what you were referring to when you said: "or even the Vercel option which generates a gigantic diff" or not. GitHub uses ncc in most (if not all) of our first-party actions.

@adamhenson
Copy link

Thanks @rachmari. Yes I tried NCC, but it threw an error in a certain project… so it seems unstable. Anyways, thanks for your responses. It’s not the end of the world… just inconvenient and not ideal especially as an open source project.

@borkdude
Copy link

Perhaps Github Actions could support downloading a zip archive of node_modules from Github releases for specific actions.

@AraHaan
Copy link

AraHaan commented Sep 17, 2022

I wish that github native dependabot could update the node_modules folder for us every time it also updates our package.json and package.lock.json.

@CarsonSlovoka
Copy link

CarsonSlovoka commented Oct 11, 2022

ncc not work too.


https://docs.github.com/en/actions/creating-actions/creating-a-javascript-action#commit-tag-and-push-your-action-to-github

They tell you

use a tool called @vercel/ncc to compile your code and modules into one file used for distribution.

and step 4:

If you already checked in your node_modules directory, remove it. rm -rf node_modules/*

when I remove node_modules/* I will get

Error: Cannot find module '@actions/core'

jhamon added a commit to pinecone-io/pinecone-python-client that referenced this issue Jun 6, 2023
## Problem

A good release process needs to be fast while also ensuring quality and
consistency. Confidence of quality comes from incorporating automated
testing. And there are many aspects of consistency including:

- Ensuring commits are tagged with version number so we can know what is
in each release (now and in the future in case of any security reports)
- Version number in `pinecone/__version__` is correctly updated, so we
don't try to reuse the same one later
- Building in a standardized way to avoid introducing accidental
inconsistencies (e.g. uncommitted and untested local changes
accidentally included in a release, using an old version of python on a
local machine, etc)

## Solution

Implement github actions workflows for publishing releases
(`release.yaml`) and prereleases (`alpha-release.yaml`).

<img width="649" alt="Screenshot 2023-06-06 at 3 01 23 PM"
src="https://github.com/pinecone-io/pinecone-python-client/assets/1326365/79e2464a-832c-447d-a771-146dc6d60ecf">

These workflows both follow the same high-level steps that take place in
a `publish-to-pypi.yaml` [reusable
workflow](https://docs.github.com/en/actions/using-workflows/reusing-workflows):
- Run tests using another new reusable workflow, `testing.yaml`, which I
refactored out of the existing PR workflow.
- Automatic version bumps based on type of release (`major`, `minor`,
`patch`).
- I ended up writing my own small action to do this because I didn't
find a nice github action that would do this off the shelf. There are
lots of small code packages out there that do the version bump logic but
they tended to have weird flaws that made them a poor fit (e.g. wanting
to handle the git commit part even though this isn't the right moment
for it in my workflow, wanting to store separate version-related config
elsewhere, poor support for custom alpha/rc version numbers, etc.)
- Check that version numbers are unique (i.e. that there is no
preexisting git tag with that number. This mainly protects us from
accidentally reusing the same rc number). This validation is possible
since git tags become the source of truth on which version numbers are
used/available.
- Build with existing make task
- Upload to PyPI using twine. This is no change, just using Makefile
tasks defined by others in the past.
- Git push commit with updated `pinecone/__version__` and git tags. Save
this step for last so that the git tag is not consumed unless the pypi
upload is successful.

## Other notes

Updated the version saved in `pinecone/__version__` because I noticed
that it is out of date with the latest version published on PyPI. This
is one example of a type of error that will no longer occur when
publishing from CI.

While implementing this I ran into several different issues in github
actions including:

- actions/runner#2472
- actions/runner#1483
- actions/hello-world-javascript-action#12


## Future Work

Some ideas for future: 
- I would like to make some automated changelog updates and draft
release notes from CI. But not doing this now as this is already a large
change.
- I could also do some work to figure out what the automatic next rc
number is by inspecting what git tags have been used. But for now it's
easy to just pass in the values as an input when starting the job.

## Type of Change

- [x] Infrastructure change (CI configs, etc)

## Test Plan

I used the test environment at test.pypi.org to publish an rc version of
this package using the `alpha-release.yaml` version of the workflow.

Here's a [test
run](https://github.com/pinecone-io/pinecone-python-client/actions/runs/5192266625)
I did that targeted the testpypi and resulted in [this 2.2.2rc4
artifact](https://test.pypi.org/project/pinecone-client/2.2.2rc4/).
Which can be installed like this by specifying the test.pypi.org index:

```
pip install -i https://test.pypi.org/simple/ pinecone-client==2.2.2rc4
```

In general, to test a github workflow that only exists in a branch you
need to install the github CLI and run a command like this from inside
the project:

```
gh workflow run alpha-release.yaml -f ref=jhamon/release-from-ci -f releaseLevel=patch -f prereleaseSuffix='rc4'
```

The only thing I changed after this run was to switch from targeting the
test PyPI index to the production PyPI index.

The prod workflow is substantially similar so I have a high confidence
it should work but I don't want to test it and release until I land this
in master because I want to the release commit to tag a SHA in the main
history (and not this branch). I plan to land, then make a follow up for
any small issues that come up when trying to use it for the first time.
@joshraker
Copy link

One of the more elegant solutions I've come across for node_modules needing to be committed to the repo of JavaScript actions is to only include them in releases. I was initially turned on to this approach by this comment which inspired this release workflow for JavaScript actions that installs packages, commits node_modules, tags the commit, and pushes the tag to the repo.

Interestingly, you can use a Docker action to install packages via the Dockerfile before running the action, as the OP was requesting. What's cooler is that, from my testing, JavaScript run inside of a Docker action container can still use the @actions/core package to read inputs, write outputs, and such for the action which IMO is a neat feature that should be documented in the GitHub Actions docs. The only downside, as others have mentioned, is that installing the packages and building the Docker image each time the action is invoked takes longer to run than including the node_modules in the repo so they can be pulled with the action.

@AraHaan
Copy link

AraHaan commented Sep 3, 2023

Yeah, doing docker is also not acceptable for me as I need my actions to complete as fast as possible.

@ncalteen
Copy link
Collaborator

Hi All, I submitted a PR to this repo to switch how this is currently handled. Instead of committing node_modules or including a npm install step before the action is run, this approach uses @vercel/ncc to package dependencies into a single JS file. This is the same approach used in the TypeScript and JavaScript action templates.

thekarel added a commit to thekarel/twist-notify that referenced this issue Oct 13, 2023
@ncalteen ncalteen self-assigned this Nov 7, 2023
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 a pull request may close this issue.