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

delete lock file when package is updated #179

Closed
wants to merge 3 commits into from
Closed

Conversation

Steve-Mcl
Copy link
Contributor

@Steve-Mcl Steve-Mcl commented Sep 27, 2023

Description

Deletes package lock when the package is updated.
This permits the internal call to npm install --production to get specified versions instead of the lock versions.

TODO:

  • test package is deleted.

Related Issue(s)

#178

Checklist

  • I have read the contribution guidelines
  • Suitable unit/system level tests have been added and they pass
  • Documentation has been updated
    • Upgrade instructions
    • Configuration details
    • Concepts
  • Changes flowforge.yml?
    • Issue/PR raised on flowforge/helm to update ConfigMap Template
    • Issue/PR raised on flowforge/CloudProject to update values for Staging/Production

Labels

  • Backport needed? -> add the backport label
  • Includes a DB migration? -> add the area:migration label

@Steve-Mcl Steve-Mcl linked an issue Sep 28, 2023 that may be closed by this pull request
10 tasks
// of the modules and therefore remove the package-lock.json file to permit modules
// to be updated to the versions specified in package.json
if (existsSync(this.files.packageLockJSON)) {
await fs.rm(this.files.packageLockJSON)
Copy link
Contributor

@Pezmc Pezmc Sep 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't an npm install already handle this?

Or are you wanting to run npm upgrade to upgrade the package lock versions specifically to the latest matching the semver? (e.g. 1.2.3 -> 1.2.5 for a ^1.2.0)

Copy link
Contributor Author

@Steve-Mcl Steve-Mcl Oct 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pez, was discussed in slack

Steve
When switching snapshots with different versions of Node-RED specified, "Installing Packages" is ran but Node-RED version remains old.
npm install --production (as the launcher and agent execute) ignore package.json and respect the lock file instead
Should we be inspecting the package or indiscriminately deleting the lock file in the agent device project dir when "Updating configuration files"-> writePackage() is called?
2 replies

nick
My instinct is to delete the package lock file whenever we update the package file

Steve
same - but wanted to check

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me, this could lead to unexpected upgrades outside of the package changes, but so would running an explicit npm upgrade, so unblocking until we find a better solution for now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought, this still doesn't fully grok for me actually, an npm install always updates the package-lock based on the latest package.json as part of the install. Testing locally.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Result of my tests in #178 (comment)

@Steve-Mcl Steve-Mcl requested review from knolleary and Pezmc October 5, 2023 15:45
@Steve-Mcl Steve-Mcl marked this pull request as ready for review October 5, 2023 15:45
@@ -54,6 +55,17 @@ class Launcher {
await fs.writeFile(this.files.packageJSON, JSON.stringify(packageData, ' ', 2))
await fs.rm(path.join(this.projectDir, '.config.nodes.json'), { force: true })
await fs.rm(path.join(this.projectDir, '.config.nodes.json.backup'), { force: true })

// as the package.json file has been re-written, we will "assume" different versions
// of the modules and therefore remove the package-lock.json file to permit modules
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be worth noting that this is only because of the --production flag, a normal npm install would update the package-lock.

Copy link
Contributor

@Pezmc Pezmc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this gets to the bottom of why the packages aren't updating, see comment on the original issue: #178 (comment)

@Steve-Mcl
Copy link
Contributor Author

@Pezmc @knolleary

Good news: I cannot recreate this from staging to device. On windows or Linux.

I can only assume some other mis-op (by me)

I will park (close) this PR (but not delete the branch for now)

@Steve-Mcl Steve-Mcl closed this Oct 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Individual Device Snapshot Selection in Developer Mode
2 participants