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

Yarn 1.x: Detect when yarn.lock has been changed #655

Closed
taylormadore opened this issue Sep 17, 2024 · 7 comments · May be fixed by #683
Closed

Yarn 1.x: Detect when yarn.lock has been changed #655

taylormadore opened this issue Sep 17, 2024 · 7 comments · May be fixed by #683
Labels
yarn Pull requests/issues related to our yarn handling module

Comments

@taylormadore
Copy link
Contributor

The yarn install command has the --frozen-lockfile flag that is supposed to fail if an update is needed to yarn.lock. Apparently needed here means if something needs to be added to the lockfile, but not if something needs to be removed. In the case of something that should be removed, Yarn will not complain and will also not update the lockfile.

We need a way to detect if yarn.lock is or would be updated by the yarn install command and fail the request.

Acceptance criteria:

  • Cachi2 detects when yarn.lock has been updated (dependencies added and/or removed) during request processing and fails the request
  • An integration test has been added
@taylormadore taylormadore added the yarn Pull requests/issues related to our yarn handling module label Sep 17, 2024
slimreaper35 added a commit to slimreaper35/cachi2 that referenced this issue Oct 14, 2024
closes containerbuildsystem#655

The integration test covers a scenario when running
`yarn install ...` command will fail becuase of new
dependencies that were added, so `yarn.lock` would
have to updated.

Signed-off-by: Michal Šoltis <[email protected]>
slimreaper35 added a commit to slimreaper35/cachi2 that referenced this issue Oct 14, 2024
closes containerbuildsystem#655

The integration test covers a scenario when running
`yarn install ...` command will fail becuase of new
dependencies that were added to package.json and yarn.lock
would have to updated to reflect these changes.

Signed-off-by: Michal Šoltis <[email protected]>
@eskultety
Copy link
Member

  • Cachi2 detects when yarn.lock has been updated (dependencies added and/or removed) during request processing and fails the request

So, if Yarn only complains if deps need to be added to the lockfile, why do we care about redundant deps if Yarn is okay with them? And if Yarn is okay with them, the user is likely okay with those as well, so we shouldn't present too many obstacles in front of the user if the native tooling isn't posing them either.

Looking at it from the other side - does NOT using --frozen-lockfile update the lockfile so that dependencies can be removed? If so and if we DO want to handle removed dependencies, then I believe we have to stop using --frozen-lockfile and implement our own detection logic, i.e.:

  • running this in a temporary clone is a must (that should be the default for us)
  • parse the lockfile before and after yarn install and compare

@taylormadore have I missed anything?

@taylormadore
Copy link
Contributor Author

why do we care about redundant deps if Yarn is okay with them?

The risk here is that we report the removed dependencies from yarn.lock in the SBOM and yet yarn does not download them to the cache

does NOT using --frozen-lockfile update the lockfile so that dependencies can be removed?

Not using --frozen-lockfile will update yarn.lock (if needed). We could either re-parse the lockfile after running yarn install without that flag or perhaps just hash the file before and after

@eskultety
Copy link
Member

The risk here is that we report the removed dependencies from yarn.lock in the SBOM

I wouldn't worry too much about the SBOM, it would match the checked-in lockfile after all (well, based on a buggy CLI behaviour of Yarn that we'd rely on), but yes, we strive for a perfect SBOM, so we could technically do better.

and yet yarn does not download them to the cache

Will this cause hermetic builds to fail? IOW if a frozen lockfile doesn't report a problem and since the online yarn install doesn't download removed dependencies to the filesystem cache, will the offline yarn install from that cache be affected in any way? Will it try to reference those dependencies somehow? Because if hermetic builds aren't affected, then the question of how much analysis vs correcting user misconfigurations do we want to perform on our perfect SBOM endeavour arises. Strictly from code perspective blaming this on Yarn and user project configuration seems like the path of least resistance while retaining much of the SBOM integrity and quality.

@eskultety
Copy link
Member

@taylormadore thoughts ^^? The discussion in this issue kinda blocks resolution and review off #683

@taylormadore
Copy link
Contributor Author

Will this cause hermetic builds to fail?

I gave it a quick test and it doesn't appear to

My feelings on this are that it isn't on the critical path, so this issue definitely doesn't need to be done now, but I'm not sure I'd just close it either. This might be something we want to do later, since it doesn't seem particularly complicated to resolve.

This is of course Just my opinion and I won't stand in the way if others feel differently 🙂

@eskultety
Copy link
Member

I gave it a quick test and it doesn't appear to.
This might be something we want to do later, since it doesn't seem particularly complicated to resolve.

Understood.

Do we want to keep issues around open? I mean at the very least we should try to resolve the issues, so if it's not pressing, we should close it for now until there's an actual use case that would need the functionality. It's not like we'll lose track of it, it can still be re-opened at any time. The problem with too many issues open makes prioritizing and triaging harder IMO.

slimreaper35 added a commit to slimreaper35/cachi2 that referenced this issue Oct 17, 2024
closes containerbuildsystem#655

The integration test covers a scenario when running
`yarn install ...` command will fail becuase of new
dependencies that were added to package.json and yarn.lock
would have to updated to reflect these changes.

Signed-off-by: Michal Šoltis <[email protected]>
slimreaper35 added a commit to slimreaper35/cachi2 that referenced this issue Oct 17, 2024
closes containerbuildsystem#655

The integration test covers a scenario when running
`yarn install ...` command will fail becuase of new
dependencies that were added to package.json and yarn.lock
would have to updated to reflect these changes.

Signed-off-by: Michal Šoltis <[email protected]>
@taylormadore
Copy link
Contributor Author

Closing it is fine with me

@taylormadore taylormadore closed this as not planned Won't fix, can't repro, duplicate, stale Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
yarn Pull requests/issues related to our yarn handling module
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants