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

Grant ownership to the package JSON file for installation step #1084

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

Conversation

SzilvasiPeter
Copy link

Motivation and Context

The change is required for local development when building docker images for testing purposes.

It fixes the #1083 issue.

Description

The npm ci is not able to run if it has no ownership over the package.json files. Similar to other COPY phase, the --chown=node:node option was added.

Contribution Checklist

  • I have built and tested the code locally and in a deployed app
  • For frontend changes, I have pulled the latest code from main, built the frontend, and committed all static files.
  • This is a change for all users of this app. No code or asset is specific to my use case or my organization.
  • I didn't break any existing functionality 😄

Copy link

@RahulVadisetty91 RahulVadisetty91 left a comment

Choose a reason for hiding this comment

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

image

There is a minor redundancy in the COPY commands related to the package*.json files. Specifically, both of these lines are doing the same task. To avoid redundancy, you can remove the first COPY command because the second one already includes the chown option, which sets the correct ownership.

The corrected code block is:

COPY --chown=node:node ./frontend/package*.json ./

@SzilvasiPeter
Copy link
Author

SzilvasiPeter commented Sep 16, 2024

Is this repository is archived or not maintained anymore? This trivial change is open for more than 10 days. Should I close the PR because it seems to me that the PR will never be merged?

@SzilvasiPeter
Copy link
Author

The redundant chown commands were removed in the 207bc6d commit.

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.

2 participants