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

Update node modules #17

Closed
wants to merge 6 commits into from
Closed

Conversation

kdorosh
Copy link

@kdorosh kdorosh commented Nov 16, 2020

updates node modules per npm install to ensure the changes in #12 are properly picked up to actually resolve #13

Fix came from our fork that is working for us. Please wait to merge until I confirm this works

@kdorosh
Copy link
Author

kdorosh commented Nov 16, 2020

passes all tests with npm run test

@kdorosh
Copy link
Author

kdorosh commented Nov 16, 2020

@rsora Tested in our ci flow, works and resolves #13

The our ci flow tested in solo-io/gloo#3874 , the successful run: https://github.com/solo-io/gloo/pull/3874/checks?check_run_id=1408007411

@kdorosh kdorosh marked this pull request as ready for review November 16, 2020 18:25
@kdorosh kdorosh changed the title WIP: Update node modules Update node modules Nov 16, 2020
@rsora
Copy link
Contributor

rsora commented Nov 16, 2020

@kdorosh thanks!
wow it seems that a lot of stuff is added to the node_modules folder, are you sure that this is all necessary?
I'm not super expert with TS, but it seems that when pulling your PR a very big amount of stuff is downloaded, maybe also dev-dependencies were downloaded during your update?

$ gh pr checkout 17  
remote: Enumerating objects: 2122, done.
remote: Counting objects: 100% (2122/2122), done.
remote: Compressing objects: 100% (1806/1806), done.
remote: Total 7692 (delta 309), reused 1997 (delta 253), pack-reused 5570
Receiving objects: 100% (7692/7692), 19.16 MiB | 4.94 MiB/s, done.
Resolving deltas: 100% (1308/1308), completed with 14 local objects.
From github.com:arduino/setup-protoc
 * [new ref]         refs/pull/17/head -> update_with_upstream
Switched to branch 'update_with_upstream'

@kdorosh
Copy link
Author

kdorosh commented Nov 16, 2020

I just ran npm install, it does seem like a lot. Perhaps I did snag dev dependencies as well. Not sure all of it is necessary.

@rsora
Copy link
Contributor

rsora commented Nov 16, 2020

I see @kdorosh, may I ask you to test again in your CI the current master branch?

I did an upgrade and a bit of dev dependencies cleanups.

In the meantime, I'm setting up a test env, but maybe you are quicker than me, I lost a bit of my grip on Github Actions 😸

@kdorosh
Copy link
Author

kdorosh commented Nov 16, 2020

current arduino master works with our ci, tested in solo-io/gloo#3875 (the passing check https://github.com/solo-io/gloo/pull/3875/checks?check_run_id=1408161055)

closing this PR since #16 handles it more cleanly

thanks @rsora !

@kdorosh kdorosh closed this Nov 16, 2020
@rsora
Copy link
Contributor

rsora commented Nov 16, 2020

Thanks @kdorosh for your support, this nice interaction reminded me why I signed to be an OSS maintainer for Arduino 👍

I'm preparing a tag and a release for the Action in the next minutes, keep your eyes peeled 😸

@per1234 per1234 added topic: infrastructure Related to project infrastructure type: imperfection Perceived defect in any part of project conclusion: duplicate Has already been submitted labels Dec 1, 2022
@per1234 per1234 self-assigned this Dec 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conclusion: duplicate Has already been submitted topic: infrastructure Related to project infrastructure type: imperfection Perceived defect in any part of project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Warning: The add-path command is deprecated and will be disabled soon
3 participants