chore: ensure nodedjs/eks dependencies are deduplicated #1559
Conversation
Does the PR have any schema changes?Looking good! No breaking changes found. |
In particular yarn may link two copies of @pulumi/pulumi into the provider causing difficult to diagnose failures. This change applies the approach from @pulumi/pulumi-awsx where the dependencies are proactively deduplicated at build time. Also note that `yarn install` should receive a --frozen-lockfile option to actually respect the lockfile during build.
t0yv0
force-pushed
the
t0yv0/dedupe-deps
branch
from
8ca467f to
6517513
Compare
| @@ -147,7 +147,8 @@ install_java_sdk:: | |||
| #Intentionally empty for CI / CD templating | |||
|
|
|||
| nodejs/eks/node_modules: nodejs/eks/package.json nodejs/eks/yarn.lock | |||
| yarn install --cwd nodejs/eks --no-progress | |||
| yarn install --frozen-lockfile --cwd nodejs/eks --no-progress | |||
There was a problem hiding this comment.
Won't this break auto-update PRs?
The behavior of yarn install without --forzen-lockfile is to do the following:
If yarn.lock is present and is enough to satisfy all the dependencies listed in package.json, the exact versions recorded in yarn.lock are installed, and yarn.lock will be unchanged. Yarn will not check for newer versions. If yarn.lock is absent, or is not enough to satisfy all the dependencies listed in package.json (for example, if you manually add a dependency to package.json), Yarn looks for the newest versions available that satisfy the constraints in package.json. The results are written to yarn.lock.
I'd say we want the lockfile to be updated when it cannot satisfy what's defined in package.json
There was a problem hiding this comment.
My intuition is that these changes should be reflected in commit history, pushing us toward hermetic builds. The default behavior is such that the checked in yarn.lock may no longer reflect the actual dependencies selected for the software.
If PRs that update dependencies modify the lock file in them I believe they will continue to work as expected with this change.
There was a problem hiding this comment.
But how would the dependency PRs update the lockfile? with --frozen-lockfile this wouldn't happen.
I don't quite understand how we could reach a point where package.json and yarn.lock would drift apart. Shouldn't the worktree clean check catch those cases?
The lock file would only be regenerated if the package.json was updated without running yarn install and if that's done, the worktree clean checks fail.
There was a problem hiding this comment.
In a Renovate PR the bot is updating yarn.lock by calling yarn without --frozen-lockfile, that is the update procedure is bypassing this Make target AFAIK.
Perhaps relevant, some places where we use frozen-lockfile:
https://github.com/search?q=org%3Apulumi%20frozen-lockfile&type=code
There was a problem hiding this comment.
You raise a good point about yarn.lock changes being detected by the worktree check. Unfortunately that does not seem to be trustworthy. First we do not do worktree checks on provider builds AFIAK but only on the SDK builds where code is expected to be generated. Then also it seems that we allow some files to drift:
- name: Check worktree clean
uses: pulumi/git-status-check-action@v1
with:
allowed-changes: |
sdk/**/pulumi-plugin.json
sdk/dotnet/Pulumi.*.csproj
sdk/go/**/pulumiUtilities.go
sdk/nodejs/package.json
sdk/python/pyproject.toml
There was a problem hiding this comment.
Shouldn't this check everything except those few allowed changes in sdk? (I.e. include provider changes like the lockfile)
Don't get me wrong, overall I agree that --frozen-lockfile is the right thing to do. I was just concerned that it will break renovate.
I didn't know that renovate automatically updates the lockfile (docs). I thought that this happens through one of our make targets.. Good to know!
There was a problem hiding this comment.
Yeah it would still catch lockfile changes it seems. But I am not sure if it is running in the same job that builds the provider.
There was a problem hiding this comment.
It is (or at least was), but the approach with the frozen lock file is definitely better! :)
In particular yarn may link two copies of @pulumi/pulumi into the provider causing difficult to diagnose failures. This change applies the approach from @pulumi/pulumi-awsx where the dependencies are proactively deduplicated at build time.
Also note that
yarn installshould receive a --frozen-lockfile option to actually respect the lockfile during build.