Skip to content

Conversation

@devabhishekpal
Copy link
Contributor

@devabhishekpal devabhishekpal commented Nov 2, 2023

What changes were proposed in this pull request?

  • The Ozone upstream repo has dependabot upgrades enabled to maintain versions for JS libraries, however right now dependabot only updates the pnpm-lock.yaml file which is an incorrect behaviour and would cause build failures.
  • This PR adds a dependabot.yml file to set the rules on which folders to scan for possible upgrades, defining the proper package manager to use, and setup general dependabot behaviour.
  • This also adds a few github actions, which would be triggered on a dependabot raised PR.
    • Why the actions are required?
      Even after we properly define the dependabot behaviour, it would properly update the versions on the dependencies. But it would also create a pnpm-lock.yaml file automatically which might not be accurate. So the github actions delete this bot generated lockfile, re-creates this file using pnpm to ensure proper generation and re-commits this proper lockfile back into the PR, ensuring the generation is properly done and there are no build issues with such an upgrade.

What is the link to the Apache JIRA

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

How was this patch tested?

This patch was tested manually.
Dummy PR was raised against the branch with the changes in the private forked repo, and then the validations were made.

Screenshot Description
Screenshot 2023-11-03 at 16 28 52 PR created by dependabot, which includes the proper version bumps in the package.json file.
HDDS-9171-dependabot-check-action dependabot-check action ran on a dummy PR raised against the branch with the changes for testing.
HDDS-9171-dependabot-check-generated-lockfile package-lock.yaml file that was created by the check action now included in the PR

@errose28 errose28 self-requested a review November 2, 2023 16:47
Copy link
Contributor

@errose28 errose28 left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @devabhishekpal. I'm not very familiar with javascript dependency management or dependabot, so I have some questions about the changes in addition to the inline comments:

  • In the image you shared, why was the pnpm lock file updated as part of a change to pom.xml?
  • 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:
[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"

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

Copy link
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

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

Thanks @devabhishekpal for working on this.

@devabhishekpal
Copy link
Contributor Author

devabhishekpal commented Nov 3, 2023

@errose28 Thanks for asking this, it is actually a good question.

So actually the main lockfile for Recon does not have this parameter.
What is happening is when dependabot updates the pnpm-lock.yaml file, it is adding this param to the lockfile, and hence the clash.
Now I am not sure why we had this not setup, but the dependabot sets it to true(ref). We do not want to auto-install peer dependencies as we might face version clash issues - it is better to resolve them manually.


Now coming to the two questions:

In the image you shared, why was the pnpm lock file updated as part of a change to pom.xml?

This is because I had actually run out of dependabot dependency PRs, so dependabot won't raise further PRs once opens an upgrade PR. So in order to test the scenario, I had manually removed the github.actor == 'dependabot[bot]' so that it runs on any PR, and then verified that the lockfile was generated. This was a dummy PR where I randomly edited the pom.xml file to trigger the actions.


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

So the current dependabot.yml change will actually properly bump the versions on the package.json, but it will also generate the pnpm-lock.yaml file, we cannot change this behaviour, and the lockfile which is generated by dependabot often times have issues, like it sets the lockfileVersion incorrectly, and sometimes other options get set like we saw for autoInstallPeerDependencies. The action will instead use pnpm to generate the lockfile, ensuring that there is no such error and the proper file is generated.

@adoroszlai
Copy link
Contributor

Thanks @devabhishekpal for updating the patch. Can you please also update the PR to reference new test runs from your fork using the updated workflow?

@devabhishekpal
Copy link
Contributor Author

Thanks @adoroszlai for the review. The following are the screenshots after the changes.

Java Package upgrades automatically raised by dependabot after change.

Screenshot 2023-11-06 at 19 52 24

Java upgrade has the dependabot-check skipped

Screenshot 2023-11-06 at 19 53 07

A dummy test PR raised with the prefix [Recon] Dependabot Package Upgrade, where the check runs

Screenshot 2023-11-06 at 19 54 13

@adoroszlai
Copy link
Contributor

@devabhishekpal Merge from master is needed only if there are conflicts, or some failing tests fixed on master are necessary to get a clean run. In this case it was unnecessary.

@adoroszlai adoroszlai changed the title HDDS 9171. Added dependabot.yml and github actions to control dependabot auto PR changes HDDS-9171. Resolve dependabot build issues when updating npm packages Nov 7, 2023
@adoroszlai
Copy link
Contributor

Thanks @devabhishekpal for the patch, I'm fine with the latest version.

@errose28 please take another look.

Copy link
Contributor

@errose28 errose28 left a comment

Choose a reason for hiding this comment

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

LGTM

@adoroszlai adoroszlai merged commit 60f6d63 into apache:master Nov 9, 2023
@adoroszlai
Copy link
Contributor

Thanks @devabhishekpal for the patch, @errose28 for the review.

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.

3 participants