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

fix: ensure api is not dropped from workspaces in package-lock.json #4623

Conversation

pichlermarc
Copy link
Member

Which problem is this PR solving?

See discussion in #4621, when releasing the API is temporarily removed. This bleeds over into package-lock.json where it's never rectified by the _restore:package-json script.

This PR adds a npm install --package-lock-only to sync the package-lock.json again after incrementing versions.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • internal

How Has This Been Tested?

  • Manually tested by running prepare_release:sdk:minor

@pichlermarc pichlermarc marked this pull request as ready for review April 10, 2024 15:41
@pichlermarc pichlermarc requested a review from a team as a code owner April 10, 2024 15:41
@@ -47,7 +47,7 @@
"comment_internal": "echo scripts below this line are for internal use",
"_check:no_changes": "if [ ! -z \"$(git status -uall --porcelain)\" ]; then echo Please ensure all changes are committed; exit 1; fi",
"_backup:package-json": "cp package.json package.json.backup",
"_restore:package-json": "mv package.json.backup package.json",
"_restore:package-json": "mv package.json.backup package.json && npm install --package-lock-only",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be less processing and safer with something like:

    "_backup:package-json": "cp package.json package.json.backup && cp package-lock.json package-lock.json.backup",
    "_restore:package-json": "mv package.json.backup package.json && mv package-lock.json.backup package-lock.json",

"safer" because with range dependencies, it is possible that npm install --package-lock-only can get a different result if there is a newer release of a dep. I'm not actually positive of that. It might depend on the state of the local npm cache. I don't know if/when npm install --package-lock-only hits the network to get new release information.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, interesting.

package-lock.json is updated when I run the lerna update script, so backing it up before and restoring after, resets it to an inconsistent state. I think we may have to do a npm install --package-lock-only regardless.

Maybe we can somehow better scope the lerna scripts. I'll try that.

Copy link
Member Author

Choose a reason for hiding this comment

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

I looked into npm install --package-lock-only and it seems that it only changes/updates things if something (package/dependency) is missing. This should never happen on main as npm ci and all workflows should be failing in such a case. Since we link everything locally, everything should already be updated on release.

I also looked into scoping lerna, and unfortunately we can't easily do this for directories as lerna --scope operates on the package name 😞

Let's try it with npm install --package-lock-only first. If we see that it's too fragile I'll look into improving it 🙂

@pichlermarc pichlermarc merged commit 87e25c5 into open-telemetry:main Apr 15, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants