Support npm publish using native npm client with .npmrc via --use-npmrc flag #2952
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fluxxBot
added
the
safe to test
github-actions
bot
removed
the
safe to test
fluxxBot
added
feature request
fluxxBot
changed the title
fluxxBot
added
the
safe to test
github-actions
bot
removed
the
safe to test
fluxxBot
added
the
safe to test
github-actions
bot
removed
the
safe to test
bhanurp
requested changes
Apr 21, 2025
Comment on lines
+1708
to
+1711
| useNpmRc: cli.BoolFlag{ | ||
| Name: useNpmRc, | ||
| Usage: "[Default: false] Set to true if you'd like to use the .npmrc file for configurations. Note: This flag would invoke npm native client behind the scenes, has performance implications and does not support deployment view and detailed summary` `", | ||
| }, |
There was a problem hiding this comment.
Suggested change
| useNpmRc: cli.BoolFlag{ | |
| Name: useNpmRc, | |
| Usage: "[Default: false] Set to true if you'd like to use the .npmrc file for configurations. Note: This flag would invoke npm native client behind the scenes, has performance implications and does not support deployment view and detailed summary` `", | |
| }, | |
| useNpmRc: cli.BoolFlag{ | |
| Name: useNPMRC, | |
| Usage: "[Default: false] Set to true to use the .npmrc file for publish. Note: This flag would invoke npm native client behind the scenes, has performance implications and does not support deployment view and detailed summary` `", | |
| }, |
| @@ -1797,8 +1800,8 @@ func GetGradleDeployedArtifacts() []string { | |||
| } | |||
| } | |||
|
|
|||
| func GetNpmDeployedScopedArtifacts(isNpm7 bool) []string { | |||
| path := NpmRepo + "/@jscope/jfrog-cli-tests/-/@jscope/" | |||
| func GetNpmDeployedScopedArtifacts(npmRepo string, isNpm7 bool) []string { | |||
There was a problem hiding this comment.
Why is this change required?
| return | ||
| } | ||
|
|
||
| func publishUsingNpmrc(configFilePath string, buildNumber string) (npm.NpmPublishCommand, error) { |
There was a problem hiding this comment.
How does sample output looks with --use-npmrc ?
| legacyCommand: "rt npm-publish", | ||
| repo: repoName, | ||
| wd: npmProjectPath, | ||
| validationFunc: validateNpmPublish, |
There was a problem hiding this comment.
validateNpmPublish is validating published module using artifact properties but --use-npmrc flag will not fail when it is not able to set properties. Is there any other way to validate the published artifact using --use-npmrc ?
| wd, err := os.Getwd() | ||
| assert.NoError(t, err, "Failed to get current dir") | ||
| defer clientTestUtils.ChangeDirAndAssert(t, wd) | ||
| buildNumber := strconv.Itoa(1) |
There was a problem hiding this comment.
Suggested change
| buildNumber := strconv.Itoa(1) | |
| buildNumber := "1" |
just wondering why not directly to string?
| addNpmScopeRegistryToNpmRc(t, npmProjectPath, packageJsonPath, npmVersion) | ||
| } | ||
|
|
||
| npmpCmd, err := publishUsingNpmrc(configFilePath, buildNumber) |
| result := npmpCmd.Result() | ||
| assert.NotNil(t, result) | ||
|
|
||
| validateNpmLocalBuildInfo(t, tests.NpmBuildName, buildNumber, moduleName) |
There was a problem hiding this comment.
Are we creating build info as well with --use-npmrc ?
| @@ -285,6 +393,14 @@ func initNpmProjectTest(t *testing.T) (npmProjectPath string) { | |||
| return | |||
| } | |||
|
|
|||
| func initNpmPublishRcProjectTest(t *testing.T, projectName string) (npmProjectPath string) { | |||
There was a problem hiding this comment.
Suggested change
| func initNpmPublishRcProjectTest(t *testing.T, projectName string) (npmProjectPath string) { | |
| func initNpmPublishviaNPMRCTest(t *testing.T, projectName string) (npmProjectPath string) { |
Comment on lines
+940
to
+941
| // we do not support deployment view and detailed summary when using npmrc for publishing since | ||
| // TransferDetails is not available to us |
There was a problem hiding this comment.
Suggested change
| // we do not support deployment view and detailed summary when using npmrc for publishing since | |
| // TransferDetails is not available to us | |
| // Deployment view and Detailed summary is not supported when using npmrc for publishing since transfer details are not available. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
devbranch.go vet ./....go fmt ./....This PR introduces a new implementation for the npm publish command that supports both the existing and the updated approach.
--use-npmrchas been added to opt into the updated publish flow..npmrcconfiguration, unlike the older approach which depended on jf config.a. npmPublishHandler: Uses the npm client.
b. rtPublishHandler: Retains the original behavior using Artifactory upload (rt).
depends on: