Skip to content

Conversation

@adoroszlai
Copy link
Contributor

@adoroszlai adoroszlai commented Apr 9, 2024

What changes were proposed in this pull request?

HDDS-9171 added a custom workflow for tweaking dependabot's PRs created for frontend dependencies. The goal was to fix the problem:

builds are failing since it is only updating the lock file

This problem no longer seems to exist, dependabot updates both pnpm-lock.yaml and package.json. Example: commit and CI run.

On the other hand, the additional commit created by the custom workflow (example) has two problems:

  1. It updates many dependencies at the same time due to lenient version definitions
    • makes PRs' actual scope much wider than originally intended
    • causes conflicts with other dependabot PRs
  2. CI is not automatically triggered, since the commit comes from automation.

Therefore I propose to remove the custom workflow.

https://issues.apache.org/jira/browse/HDDS-10668

How was this patch tested?

N/A

@adoroszlai adoroszlai added the CI label Apr 9, 2024
@adoroszlai adoroszlai self-assigned this Apr 9, 2024
@adoroszlai adoroszlai requested a review from errose28 April 9, 2024 07:59
@errose28
Copy link
Contributor

In the time since #5538 I've had to learn how pnpm works more or less for the new Ozone website, so I'm pretty sure I can explain what happened here. This is still not an area I have much experience in so please double check for yourself.

Prior to #5538, we would see dependabot PRs failing in the build step with this error:

    [INFO] --- frontend-maven-plugin:1.12.0:npx (install frontend dependencies) @ ozone-recon ---
    [INFO] npm not inheriting proxy config from Maven
    [INFO] Running 'npx [email protected] install --frozen-lockfile' in /home/runner/work/ozone/ozone/hadoop-ozone/recon/src/main/resources/webapps/recon/ozone-recon-web
    [INFO]  ERR_PNPM_LOCKFILE_CONFIG_MISMATCH  Cannot proceed with the frozen installation. The current "settings.autoInstallPeers" configuration doesn't match the value found in the lockfile
    [INFO] 
    [INFO] Update your lockfile using "pnpm install --no-frozen-lockfile"

In the version of the code where we would see this error, we did not explicitly define a pnpm version in package.json. Our pnpm version was still pinned for stability, but it is pinned in pom.xml since #2115 because we have a somewhat atypical build where Maven, our primary build system, then invokes pnpm as a secondary build sytem.

Dependabot supports both pnpm v7 and v8. If the project does explicitly define a version in package.json, it will default to v8, the latest. Our atypical build structure did not pin the pnpm version in the typical place in package.json, so dependabot generated the lockfile with pnpm v8. Then our build job would come in after and run on this lockfile with pnpm v7 pinned in pom.xml.

The error we were seeing is due to a difference in pnpm v7 and v8. In v8, autoInstallPeers defaults to true. This would cause dependabot to add these lines to the top of the pnpm-lock.yaml file it generated:

settings:
	autoInstallPeers: true
	excludeLinksFromLockfile: false

However, in v7, autoInstallPeers defaults to false. The build runs with --frozen-lockfile as shown above, so when this config in the lockfile generated by dependabot (true) does not match the one pnpm v7 is running with (false), it fails.

The solution is just to pin pnpm to a v7 version in package.json where dependabot will see it. This way dependabot generates the lockfile with the same version we will use to build it in the job. So #5538 contained the fix, but it was not the extra dependabot workflow, it was this single line.

I was kind of on to this with this comment, but at the time I didn't have enough pnpm knowledge to make the case that the extra workflow was unnecessary, other than that it seemed lots of other people were using pnpm and dependabot without needing it so there must be a simpler fix. For posterity I will answer my own questions now:

  • Is there a way to configure dependabot to work correctly without needing a custom action? I don't totally understand the current error before this PR or if there is a config to fix this:

I explained the error above, and yes there is a one line config to fix this problem.

Maybe something like these could fix it?
pnpm/pnpm#6649
https://github.com/orgs/pnpm/discussions/6633

Yes both of those threads mention pinning your pnpm version so that dependabot uses the same version as the repo's build, which will fix the problem.

All of that to say that code wise this change LGTM since we are now pinning the pnpm version in package.json and the dependabot workflow isn't the actual fix. Based on the PR description I think the reason this PR works is different than we initially thought. There is also the caveat that we have pnpm version pinned in package.json and pom.xml and have to manually keep them in sync.

@adoroszlai adoroszlai merged commit 0c59c18 into apache:master Apr 11, 2024
@adoroszlai adoroszlai deleted the HDDS-10668 branch April 11, 2024 06:37
@adoroszlai
Copy link
Contributor Author

Thanks @errose28 for the review and explanation about pnpm.

@errose28
Copy link
Contributor

Thanks for the fix and keeping an eye on the dependabot issues @adoroszlai. It's easy for these kinds of things to get neglected.

Tejaskriya pushed a commit to Tejaskriya/ozone that referenced this pull request Apr 17, 2024
jojochuang pushed a commit to jojochuang/ozone that referenced this pull request May 29, 2024
adoroszlai added a commit to adoroszlai/ozone that referenced this pull request Sep 25, 2025
(cherry picked from commit 0c59c18)
(cherry picked from commit fc401bca5cb4bfbf5f5515f2b804dcb74328aae7)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants