You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/12/07 09:31:38 UTC
[GitHub] [pulsar-client-node] shibd opened a new pull request, #258: Remove unseless dependencies.
shibd opened a new pull request, #258:
URL: https://github.com/apache/pulsar-client-node/pull/258
### Motivation
In #235, wrong introduction of verbose dependencies. It will cause the installation to fail using `yarn install`.
### Modifications
- Remove `verbose` dependence.
- Remove `node-pre-gyp-github`, is also useless dependence. It is responsible for publishing NAPI to Release page. Refer #245; We don't need to publish.
### Verifying this change
- [ ] Make sure that the change passes the CI checks.
*(Please pick either of the following options)*
This change is a trivial rework / code cleanup without any test coverage.
*(or)*
This change is already covered by existing tests, such as *(please describe tests)*.
*(or)*
This change added tests and can be verified as follows:
*(example:)*
- *Added integration tests for end-to-end deployment with large payloads (10MB)*
- *Extended integration test for recovery after broker failure*
### Documentation
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `doc-required`
(Your PR needs to update docs and you will update later)
- [x] `doc-not-needed`
(Please explain why)
- [ ] `doc`
(Your PR contains doc changes)
- [ ] `doc-complete`
(Docs have been already added)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-node] BewareMyPower commented on a diff in pull request #258: Remove useless dependencies.
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #258:
URL: https://github.com/apache/pulsar-client-node/pull/258#discussion_r1042097796
##########
.github/workflows/ci-pr-validation.yml:
##########
@@ -90,8 +90,8 @@ jobs:
- name: Build Node binaries lib
run: |
yarn install --ignore-scripts
Review Comment:
Could you verify both `yarn install` and `npm install`?
```bash
echo "Verifying yarn install"
yarn install --ignore-scripts
# Maybe removing the `node_modules` directory is unnecessary
rm -rf node_modules/
echo "Verifying npm install"
npm install --ignore-scripts
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-node] shibd commented on pull request #258: Remove useless dependencies.
Posted by GitBox <gi...@apache.org>.
shibd commented on PR #258:
URL: https://github.com/apache/pulsar-client-node/pull/258#issuecomment-1342579551
> Not sure whether should we add two lock files for both `npm` and `yarn`. When I ran `yarn install` in my local env, there was a warning:
>
> ```shell
> $ yarn install --no-scripts
> yarn install v1.22.19
> warning package-lock.json found. Your project contains lock files generated by tools other than Yarn. It is advised not to mix package managers in order to avoid resolution inconsistencies caused by unsynchronized lock files. To clear this warning, remove package-lock.json.
> ```
>
> After the `yarn install` command is done, the `yarn.lock` file changed a lot:
>
> ```shell
> $ git diff yarn.lock | wc -l
> 7082
> ```
You are right. For developers, we should only use one way to manage dependencies. I remove `yarn.lock`.
BTW: For users, It can optionally use `yarn` or `npm`. So, currently, we are not uploading package.lock.json to the npm repository.
In this PR, we verified the installation method using yarn.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-node] shibd commented on a diff in pull request #258: Remove useless dependencies.
Posted by GitBox <gi...@apache.org>.
shibd commented on code in PR #258:
URL: https://github.com/apache/pulsar-client-node/pull/258#discussion_r1043257736
##########
.github/workflows/ci-pr-validation.yml:
##########
@@ -90,8 +90,8 @@ jobs:
- name: Build Node binaries lib
run: |
yarn install --ignore-scripts
Review Comment:
> Thanks for the reminder, should change to cache yarn here.
@tisonkun I didn't change to using `yarn` cache because using `yarn` cache requires a `yarn.lock` file. Referring to this [comment](https://github.com/apache/pulsar-client-node/pull/258#issuecomment-1342579551), we don't add yarn.lock.
For this compilation step, I chose not to use a cache.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-node] shibd commented on a diff in pull request #258: Remove useless dependencies.
Posted by GitBox <gi...@apache.org>.
shibd commented on code in PR #258:
URL: https://github.com/apache/pulsar-client-node/pull/258#discussion_r1042219878
##########
.github/workflows/ci-pr-validation.yml:
##########
@@ -90,8 +90,8 @@ jobs:
- name: Build Node binaries lib
run: |
yarn install --ignore-scripts
Review Comment:
> Could you verify both yarn install and npm install?
In fact, used `npm install` when building windows and Linux Arch. Then, I changed to using `yarn install` when compiling macOS. This has verified that both yarn and npm can be used.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-node] BewareMyPower commented on a diff in pull request #258: Remove useless dependencies.
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #258:
URL: https://github.com/apache/pulsar-client-node/pull/258#discussion_r1042850341
##########
.github/workflows/ci-pr-validation.yml:
##########
@@ -90,8 +90,8 @@ jobs:
- name: Build Node binaries lib
run: |
yarn install --ignore-scripts
Review Comment:
> Any motivation to make this change?
@tisonkun Because there is a weird issue that `yarn install` failed for the current master while `npm install` succeeded.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-node] BewareMyPower commented on a diff in pull request #258: Remove useless dependencies.
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #258:
URL: https://github.com/apache/pulsar-client-node/pull/258#discussion_r1043351018
##########
.github/workflows/ci-pr-validation.yml:
##########
@@ -58,7 +58,6 @@ jobs:
uses: actions/setup-node@v3
with:
node-version: ${{ matrix.nodejs }}
- cache: 'npm'
Review Comment:
The workflow is executed concurrently. And it would not take too much time. See the x64 build for Node 18 on Linux, it only takes 5 minutes in total.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-node] shibd commented on a diff in pull request #258: Remove useless dependencies.
Posted by GitBox <gi...@apache.org>.
shibd commented on code in PR #258:
URL: https://github.com/apache/pulsar-client-node/pull/258#discussion_r1043375829
##########
.github/workflows/ci-pr-validation.yml:
##########
@@ -58,7 +58,6 @@ jobs:
uses: actions/setup-node@v3
with:
node-version: ${{ matrix.nodejs }}
- cache: 'npm'
Review Comment:
Ok, Changed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-node] shibd commented on pull request #258: Remove useless dependencies.
Posted by GitBox <gi...@apache.org>.
shibd commented on PR #258:
URL: https://github.com/apache/pulsar-client-node/pull/258#issuecomment-1346670923
@tisonkun @RobertIndie @BewareMyPower PTAL.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-node] tisonkun commented on a diff in pull request #258: Remove useless dependencies.
Posted by GitBox <gi...@apache.org>.
tisonkun commented on code in PR #258:
URL: https://github.com/apache/pulsar-client-node/pull/258#discussion_r1043266963
##########
.github/workflows/ci-pr-validation.yml:
##########
@@ -58,7 +58,6 @@ jobs:
uses: actions/setup-node@v3
with:
node-version: ${{ matrix.nodejs }}
- cache: 'npm'
Review Comment:
If we choose to check in the npm lock file, it's strange that we run CI tasks with `yarn`.
What if moving the pkg manager to `yarn` totally?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-node] shibd commented on a diff in pull request #258: Remove useless dependencies.
Posted by GitBox <gi...@apache.org>.
shibd commented on code in PR #258:
URL: https://github.com/apache/pulsar-client-node/pull/258#discussion_r1042219878
##########
.github/workflows/ci-pr-validation.yml:
##########
@@ -90,8 +90,8 @@ jobs:
- name: Build Node binaries lib
run: |
yarn install --ignore-scripts
Review Comment:
> Could you verify both yarn install and npm install?
In fact, used `npm install` when building Windows as well as Linux Arch. This has verified that both yarn and npm can be used.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-node] shibd commented on pull request #258: Remove useless dependencies.
Posted by GitBox <gi...@apache.org>.
shibd commented on PR #258:
URL: https://github.com/apache/pulsar-client-node/pull/258#issuecomment-1341021337
@BewareMyPower @RobertIndie @tisonkun I add `yarn.lock` to make sure developers who compile with yarn using the same dependent version.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-node] BewareMyPower commented on a diff in pull request #258: Remove useless dependencies.
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #258:
URL: https://github.com/apache/pulsar-client-node/pull/258#discussion_r1043288903
##########
.github/workflows/ci-pr-validation.yml:
##########
@@ -58,7 +58,6 @@ jobs:
uses: actions/setup-node@v3
with:
node-version: ${{ matrix.nodejs }}
- cache: 'npm'
Review Comment:
> What if moving the pkg manager to yarn totally?
I think we'd better not. The motivation is to prevent the regression happened again. I think we can add a separated workflow to verify the `yarn install` in a single env like Linux.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-node] shibd commented on a diff in pull request #258: Remove useless dependencies.
Posted by GitBox <gi...@apache.org>.
shibd commented on code in PR #258:
URL: https://github.com/apache/pulsar-client-node/pull/258#discussion_r1042231727
##########
.github/workflows/ci-pr-validation.yml:
##########
@@ -89,9 +89,9 @@ jobs:
- name: Build Node binaries lib
run: |
- npm install --ignore-scripts
- npx node-pre-gyp configure --target_arch=${{ matrix.arch }}
- npx node-pre-gyp build --target_arch=${{ matrix.arch }}
+ yarn install --ignore-scripts
Review Comment:
In fact, used npm install when building Windows as well as Linux Arch. This has verified that both yarn and npm can be used.
I'll keep using `yarn` when compiling macOS.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-node] shibd commented on a diff in pull request #258: Remove useless dependencies.
Posted by GitBox <gi...@apache.org>.
shibd commented on code in PR #258:
URL: https://github.com/apache/pulsar-client-node/pull/258#discussion_r1042231727
##########
.github/workflows/ci-pr-validation.yml:
##########
@@ -89,9 +89,9 @@ jobs:
- name: Build Node binaries lib
run: |
- npm install --ignore-scripts
- npx node-pre-gyp configure --target_arch=${{ matrix.arch }}
- npx node-pre-gyp build --target_arch=${{ matrix.arch }}
+ yarn install --ignore-scripts
Review Comment:
In fact, used npm install when building windows and Linux Arch. Then, I changed to using `yarn` when compiling macOS. This has verified that both yarn and npm can be used.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-node] BewareMyPower merged pull request #258: Remove useless dependencies.
Posted by GitBox <gi...@apache.org>.
BewareMyPower merged PR #258:
URL: https://github.com/apache/pulsar-client-node/pull/258
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-node] tisonkun commented on a diff in pull request #258: Remove useless dependencies.
Posted by GitBox <gi...@apache.org>.
tisonkun commented on code in PR #258:
URL: https://github.com/apache/pulsar-client-node/pull/258#discussion_r1042115030
##########
.github/workflows/ci-pr-validation.yml:
##########
@@ -90,8 +90,8 @@ jobs:
- name: Build Node binaries lib
run: |
yarn install --ignore-scripts
Review Comment:
Since we only cache for `npm` above:
```
cache: 'npm'
```
I don't think we should update the pkg manager here. Any motivation to make this change? If so, it can hide some subtle bugs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-node] shibd commented on a diff in pull request #258: Remove useless dependencies.
Posted by GitBox <gi...@apache.org>.
shibd commented on code in PR #258:
URL: https://github.com/apache/pulsar-client-node/pull/258#discussion_r1043295599
##########
.github/workflows/ci-pr-validation.yml:
##########
@@ -58,7 +58,6 @@ jobs:
uses: actions/setup-node@v3
with:
node-version: ${{ matrix.nodejs }}
- cache: 'npm'
Review Comment:
> I think we'd better not. The motivation is to prevent the regression happened again. I think we can add a separated workflow to verify the yarn install in a single env like Linux.
Reuse the current workflow to save CI execution time. `yarn install` command can take tens of seconds.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-node] shibd commented on a diff in pull request #258: Remove useless dependencies.
Posted by GitBox <gi...@apache.org>.
shibd commented on code in PR #258:
URL: https://github.com/apache/pulsar-client-node/pull/258#discussion_r1043293220
##########
.github/workflows/ci-pr-validation.yml:
##########
@@ -58,7 +58,6 @@ jobs:
uses: actions/setup-node@v3
with:
node-version: ${{ matrix.nodejs }}
- cache: 'npm'
Review Comment:
> If we choose to check in the npm lock file, it's strange that we run CI tasks with yarn.
For users, when the user execution `npm install pulsar-client` or `yarn add pulsar-client`. they are not using pulsr-clien's npm lock file(we are not uploading package.lock.json to the npm registry).
We are experiencing an issue where users can install successfully with npm but fail with yarn. So, The use of yarn here is to verify the scene of `pulsar-client` installed by the user using `yarn`.
> What if moving the pkg manager to yarn totally?
If so, just in case. We also need to verify the installation using npm.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-node] shibd commented on a diff in pull request #258: Remove useless dependencies.
Posted by GitBox <gi...@apache.org>.
shibd commented on code in PR #258:
URL: https://github.com/apache/pulsar-client-node/pull/258#discussion_r1042230580
##########
.github/workflows/ci-pr-validation.yml:
##########
@@ -90,8 +90,8 @@ jobs:
- name: Build Node binaries lib
run: |
yarn install --ignore-scripts
Review Comment:
> Any motivation to make this change?
Please refer this PR `Motivation`
> Since we only cache for npm above:
Thanks for the reminder, should change to cache yarn here.
##########
.github/workflows/ci-pr-validation.yml:
##########
@@ -90,8 +90,8 @@ jobs:
- name: Build Node binaries lib
run: |
yarn install --ignore-scripts
Review Comment:
> Any motivation to make this change?
Please refer this PR `Motivation`
> Since we only cache for npm above:
Thanks for the reminder, should change to cache yarn here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-node] RobertIndie commented on a diff in pull request #258: Remove unseless dependencies.
Posted by GitBox <gi...@apache.org>.
RobertIndie commented on code in PR #258:
URL: https://github.com/apache/pulsar-client-node/pull/258#discussion_r1042071379
##########
.github/workflows/ci-pr-validation.yml:
##########
@@ -89,9 +89,9 @@ jobs:
- name: Build Node binaries lib
run: |
- npm install --ignore-scripts
- npx node-pre-gyp configure --target_arch=${{ matrix.arch }}
- npx node-pre-gyp build --target_arch=${{ matrix.arch }}
+ yarn install --ignore-scripts
Review Comment:
We need to verify using both yarn and npm
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-node] shibd commented on a diff in pull request #258: Remove unseless dependencies.
Posted by GitBox <gi...@apache.org>.
shibd commented on code in PR #258:
URL: https://github.com/apache/pulsar-client-node/pull/258#discussion_r1042078871
##########
.github/workflows/ci-pr-validation.yml:
##########
@@ -89,9 +89,9 @@ jobs:
- name: Build Node binaries lib
run: |
- npm install --ignore-scripts
- npx node-pre-gyp configure --target_arch=${{ matrix.arch }}
- npx node-pre-gyp build --target_arch=${{ matrix.arch }}
+ yarn install --ignore-scripts
Review Comment:
fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-node] shibd commented on a diff in pull request #258: Remove useless dependencies.
Posted by GitBox <gi...@apache.org>.
shibd commented on code in PR #258:
URL: https://github.com/apache/pulsar-client-node/pull/258#discussion_r1043258033
##########
.github/workflows/ci-pr-validation.yml:
##########
@@ -58,7 +58,6 @@ jobs:
uses: actions/setup-node@v3
with:
node-version: ${{ matrix.nodejs }}
- cache: 'npm'
Review Comment:
@tisonkun I didn't change to using yarn cache because using yarn cache requires a yarn.lock file. Referring to this https://github.com/apache/pulsar-client-node/pull/258#issuecomment-1342579551, we don't add yarn.lock.
For this compilation step, I chose not to use a cache.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org