Skip to content

feat: verify package-lock.json UTD (up to date) #4598

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

Open
wants to merge 30 commits into
base: main
Choose a base branch
from

Conversation

mfranzke
Copy link
Collaborator

Proposed changes

From time to time we experience out of date package-lock.json files. To ensure that those aren't resulting out of our local development (as they are unlikely based on dependabot updates), we should ensure that those are quickly checked in git push.

Types of changes

  • Bugfix (non-breaking change that fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Refactoring (improvements to existing components or architectural decisions)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation Update (if none of the other choices apply)

Further comments

@mfranzke mfranzke self-assigned this Jul 22, 2025
@mfranzke mfranzke requested a review from nmerget as a code owner July 22, 2025 07:25
@mfranzke mfranzke added the 🍄🆙improvement New feature or request label Jul 22, 2025
@mfranzke mfranzke moved this to 👀 In review in UX Engineering Team Backlog Jul 22, 2025
@mfranzke mfranzke removed their assignment Jul 22, 2025
@mfranzke mfranzke enabled auto-merge (squash) July 22, 2025 07:45
@mfranzke mfranzke requested a review from Copilot July 22, 2025 14:43
Copilot

This comment was marked as outdated.

@mfranzke mfranzke added the 🍒 cherryPick Code that we should adapt from one repository to another. label Jul 23, 2025
@nmerget
Copy link
Collaborator

nmerget commented Jul 23, 2025

I think this isn't a good solution.

The package you used just uses npm install --package-lock-only --ignore-scripts to compare the file contents. If we switch to pnpm this package wouldn't work.

Anyways I don't see the problem with npm ci. The first step in our pipeline would fail if the package-lock.json is out of date. This happens sometimes, but most of the time you have to install the dependency to use it in you branch.

Instead of adding this we should consider solving #3372 and move to pnpm. A lot of current problems would be solved with pnpm

@mfranzke
Copy link
Collaborator Author

I think this isn't a good solution.

The package you used just uses npm install --package-lock-only --ignore-scripts to compare the file contents. If we switch to pnpm this package wouldn't work.

Anyways I don't see the problem with npm ci. The first step in our pipeline would fail if the package-lock.json is out of date. This happens sometimes, but most of the time you have to install the dependency to use it in you branch.

Instead of adding this we should consider solving #3372 and move to pnpm. A lot of current problems would be solved with pnpm

I mainly agree on wanting to proceed with using the package, but do like the idea of an additional check previous to a push, as we had some more npm ci fails in the pipelines previously, which would have been prevented by that conditional qa-test on our localhosts, that doesn't hurt anybody.

@mfranzke mfranzke requested a review from Copilot July 23, 2025 09:22
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds a pre-push Git hook to automatically verify that package-lock.json files are up to date before pushing changes. The hook detects changes to package files and runs npm install --package-lock-only to ensure the lock file is synchronized with package.json.

  • Implements automated verification of package-lock.json synchronization during git push
  • Uses a configurable pattern-matching system to detect relevant file changes
  • Prevents pushes when package files are out of sync

Comment on lines +35 to +36
# Exit after first match to avoid running multiple commands
exit 0
Copy link
Preview

Copilot AI Jul 23, 2025

Choose a reason for hiding this comment

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

The script exits with code 0 after running the first matching command, which will allow the push to proceed even if the command was meant to modify files (like updating package-lock.json). The script should check if files were modified and either fail the push or stage the changes appropriately.

Suggested change
# Exit after first match to avoid running multiple commands
exit 0
# Check for file modifications after running the command
MODIFIED_FILES=$(git diff --name-only)
if [ -n "$MODIFIED_FILES" ]; then
echo "Detected file modifications after running $command:"
echo "$MODIFIED_FILES"
echo "Please stage the changes before pushing."
exit 1
fi

Copilot uses AI. Check for mistakes.

echo "Detected changes in $description"

## Run the corresponding command
eval "$command"
Copy link
Preview

Copilot AI Jul 23, 2025

Choose a reason for hiding this comment

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

Using eval with user-configurable commands poses a security risk. Consider using a safer alternative like directly calling the command or implementing a whitelist of allowed commands.

Suggested change
eval "$command"
bash -c "$command"

Copilot uses AI. Check for mistakes.

Comment on lines +19 to +22
for ((i=0; i<${#CHECKS[@]}; i+=3)); do
pattern="${CHECKS[i]}"
command="${CHECKS[i+1]}"
description="${CHECKS[i+2]}"
Copy link
Preview

Copilot AI Jul 23, 2025

Choose a reason for hiding this comment

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

The array iteration logic with manual index incrementing by 3 is error-prone and hard to maintain. Consider using associative arrays or a more structured data format to improve readability and reduce the risk of indexing errors.

Suggested change
for ((i=0; i<${#CHECKS[@]}; i+=3)); do
pattern="${CHECKS[i]}"
command="${CHECKS[i+1]}"
description="${CHECKS[i+2]}"
for pattern in "${!CHECKS[@]}"; do
IFS='|' read -r command description <<< "${CHECKS[$pattern]}"

Copilot uses AI. Check for mistakes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🍒 cherryPick Code that we should adapt from one repository to another. 🛠️configuration 🍄🆙improvement New feature or request
Projects
Status: 👀 In review
Development

Successfully merging this pull request may close these issues.

2 participants