-
Notifications
You must be signed in to change notification settings - Fork 157
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
Correct .npmrc for newer versions of NPM #787
base: npm-8.19
Are you sure you want to change the base?
Conversation
Include Version compare for NpmBuildInfoExtractor
Added update to Auth prop for newer NPM versions
Info corrections
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
I have read the CLA Document and I hereby sign the CLA |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for creating this PR!
See my inline comments.
I'd like to take this opportunity and recommend using JFrog CLI for integrating with npm, rather than this library. Contrary to this library, the codebase of JFrog CLI receieves a lot more attention, more features and improvements and is signifficantly more popular.
build-info-extractor-npm/src/main/java/org/jfrog/build/extractor/npm/NpmDriver.java
Outdated
Show resolved
Hide resolved
...tractor-npm/src/main/java/org/jfrog/build/extractor/npm/extractor/NpmBuildInfoExtractor.java
Outdated
Show resolved
Hide resolved
I'm not part of the dev-ops team, but i'll mention to them that this Plug-in is not being as actively developed. As mentioned, the part where this should shine is with running against Node 15 and Node 20, where as before that would have failed. |
@eyalbe4 Any chance you can re-run the checks? |
@eyalbe4 Is there anything else needing to be done for this to be rechecked? |
@yahavi Are you able to re-execute the checks? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
@Spaction , Sorry for the delay, we'll review this soon |
This reverts commit f0f8328.
Hi @Spaction , Seems like the npm tests are failing. Could you please look into it before we proceed? |
Correct Unit Test
Updated the Unit tests. Can honestly say the test doesn't differ between between the NPMExtractor Test vs the one for NpmBuildInfo. The expanded test for the v15 vs v20 is what would have failed previously during the NPMExtractorTest. Only way to confirm this would be to expand only the unit test to include Node v20 and see that it fails for that branch. This is only to say that the new test can be excluded as the other indirectly tests the method\execution path in its npmCiTest |
@RobiNino Any updates on my last response? |
@Spaction, |
@eyalbe4 You mentioned that back in March, to which I'll repeat what I said earlier. I've no control over what tools we use. I've brought up to the team responsible to use the newer library and for reasons I won't state here was told it would not be happening. |
Replaces old _auth from the temp npmrc file conditionally if we are past or equal the version 8.19 which introduced the new format.