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

Add NPM autodiscovery #1065

Merged
merged 26 commits into from
Feb 1, 2023
Merged

Add NPM autodiscovery #1065

merged 26 commits into from
Feb 1, 2023

Conversation

olblak
Copy link
Member

@olblak olblak commented Jan 1, 2023

Signed-off-by: Olblak me@olblak.com

Dependson:

Add npm autodiscovery to bump any version specified as dependencies or devDependencies specified in a package.json

Updatecli ignores version bump for the following scenarios

  • any package.json in node_modules directories
  • Any version constraint handled by NPM starting with "*", ">","<","~"

Example

Manifest

Click to expand

########################################
# [NPM] BUMP @MDI/FONT PACKAGE VERSION #
########################################

name: '[NPM] Bump @mdi/font package version'
pipelineid: b1f2a3e1656341a003847d1b725c14081e2152f27166a31bd668902bc7814d15
sources:
    npm:
        name: Get @mdi/font package version
        kind: npm
        spec:
            name: '@mdi/font'
            versionfilter:
                kind: semver
                pattern: '>=5.9.55'
targets:
    npm:
        name: Bump @mdi/font package version
        kind: json
        spec:
            file: package.json
            key: dependencies.@mdi/font
        sourceid: npm

Pipeline Run

Click to expand
 ~/Projects/Updatecli/app-dashboard │ ../updatecli/bin/updatecli diff --experimental                                                                                       
Experimental Mode Enabled


+++++++++++
+ PREPARE +
+++++++++++


SCM repository retrieved: 0


++++++++++++++++++
+ AUTO DISCOVERY +
++++++++++++++++++



#######################
# LOCAL AUTODISCOVERY #
#######################

NPM
====
1 manifests identified
Manifest detected: 1

---

=> Total manifest detected: 1



++++++++++++
+ PIPELINE +
++++++++++++



#######################
# LOCAL AUTODISCOVERY #
#######################


########################################
# [NPM] BUMP @MDI/FONT PACKAGE VERSION #
########################################


SOURCES
=======

npm
---
Searching for version matching pattern ">=5.9.55"
✔ Version 7.1.96 found for package name "@mdi/font"


TARGETS
========

npm
---

**Dry Run enabled**

⚠ Key 'dependencies.@mdi/font', from file 'package.json', is incorrectly set to 5.9.55 and should be 7.1.96'

=============================

REPORTS:


✔ Local AutoDiscovery:

⚠ [NPM] Bump @mdi/font package version:
	Source:
		✔ [npm] Get @mdi/font package version (kind: npm)
	Target:
		⚠ [npm] Bump @mdi/font package version (kind: json)


Run Summary
===========
Pipeline(s) run:
  * Changed:	1
  * Failed:	0
  * Skipped:	0
  * Succeeded:	1
  * Total:	2

Test

To test this pull request, you can run the following commands:

go build .
# Then from any directories containing package.json, run
updatecli diff --experimental 
# OR
cd pkg/plugins/autodiscovery/npm/
go test
cd test 
go test

Additional Information

Tradeoff

Potential improvement

  1. I know have a lot of logic in the npm autodiscovery plugin that should be moved to a specific npm resource
  2. Allow to restrict version update to a specific type such as only major/minor/patch update e if semver is used
  3. Yarn Target updating the lockfile, when executed in dry-run mode shouldn't not change the yarn.lock`
  4. Leverage google/osv.dev to detect CVEs

Signed-off-by: Olblak <me@olblak.com>
@olblak
Copy link
Member Author

olblak commented Jan 1, 2023

Well in fact it should ignore any non semver version https://docs.npmjs.com/cli/v6/configuring-npm/package-json#dependencies

@olblak
Copy link
Member Author

olblak commented Jan 2, 2023

npm package allows dots in name such as "highlight.js" which is not compatible with the current json key.
I need to additional testing

@olblak
Copy link
Member Author

olblak commented Jan 2, 2023

After a night thinking on this, I realize that I took a different path than Dependabot.
In this pullrequest, I assume that a version constraint specified in a package.json shouldn't be changed.
Differently said, >2.17 should not be changed by Updatecli unless we specify the flag force (not implemented yet).

On the other side, Dependabot proposes a version bump following the same version pattern so it would propose >2.18 to replace 2.17 if possible.

I do prefer to assume that a version constraint specified in a package.json has a meaning that Updatecli shouldn't override

Signed-off-by: Olblak <me@olblak.com>
Signed-off-by: Olblak <me@olblak.com>
Signed-off-by: Olblak <me@olblak.com>
…ailable

Signed-off-by: Olblak <me@olblak.com>
@olblak
Copy link
Member Author

olblak commented Jan 3, 2023

I improved the crawler to add an additional target to cleanup the lock file generated by yarn or npm.
If a lockfile exists then Updatecli requires yarn or npm to be executed

targets:
    npm:
        name: Bump "@mdi/font" package version
        kind: json
        spec:
            file: package.json
            key: dependencies.@mdi/font
        scmid: default
        sourceid: npm
    package-lock.json:
        dependson:
            - npm
        name: Update NPM lockfile package-lock.json
        kind: shell
        spec:
            command: npm install --package-lock-only
        scmid: default
        disablesourceinput: true

I am now facing two additional issues

  1. The npm command must be executed from the same directory than the package.json but the shell resource do not allow to override the default workingdirectory. One option would be to add an additional shell parameter "workingdir"
  2. The shell command is always executed and therefor try to commit unchanged files which trigger the same errror than Add Git Branch resource #1000 (comment)

Working on #942 could solve the current issues

@olblak olblak added this to the 0.46.0 milestone Jan 23, 2023
@olblak olblak mentioned this pull request Jan 24, 2023
2 tasks
@olblak olblak added autodiscovery All things related to the autodiscovery feature enhancement New feature or request labels Jan 26, 2023
@olblak
Copy link
Member Author

olblak commented Jan 30, 2023

Update on my testing of this pullrequest.

The "file/checksum" shell success criteria doesn't fully solve my problem. As it checks if a file changed or not, in dry-run mode, Updatecli do not detect if a file should be changed even though the console output correctly display a change such as

Example

##################################
# BUMP "CORE-JS" PACKAGE VERSION #
##################################


SOURCES
=======

npm
---
Searching for version matching pattern "^3.8.0"
✔ Version 3.27.2 found for package name "core-js"


TARGETS
========

package-lock.json
-----------------

**Dry Run enabled**

The shell 🐚 command "/bin/sh /tmp/updatecli/bin/34fde7d5e0d61b4cf2a5aedcdd9c1aebb51d5d97ed959020c8d6ebfc1cc2c778.sh" ran successfully with the following output:
----

up to date in 718ms

103 packages are looking for funding
  run `npm fund` for details
----
No change detected

Signed-off-by: Olblak <me@olblak.com>
Signed-off-by: Olblak <me@olblak.com>
Signed-off-by: Olblak <me@olblak.com>
olblak and others added 8 commits January 30, 2023 14:53
Signed-off-by: Olblak <me@olblak.com>
Signed-off-by: Olblak <me@olblak.com>
Signed-off-by: Olblak <me@olblak.com>
Signed-off-by: Olblak <me@olblak.com>
Signed-off-by: Olblak <me@olblak.com>
@olblak olblak modified the milestones: 0.46.0, 0.44.0 Feb 1, 2023
@olblak olblak merged commit fcef1d9 into updatecli:main Feb 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autodiscovery All things related to the autodiscovery feature enhancement New feature or request resource-npm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants