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

Add option to ignore out of sync lock files #49

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

Conversation

MaelGNM
Copy link

@MaelGNM MaelGNM commented Mar 1, 2023

What does this change?

Because of how the support dotcom components packages are structured the snyk task fails with the following error:

OutOfSyncError: Dependency @sdc/shared was not found in yarn.lock. Your package.json and yarn.lock are probably out of sync. Please run "yarn install" and try again.

This PR adds support for the --strict-out-of-sync option for snyk monitor.
It adds a new optional boolean parameter to this task name IGNORE_OUT_OF_SYNC, if set to true it sets adds the option --strict-out-of-sync=false to the snyk command.

The default for --strict-out-of-sync is true so setting IGNORE_OUT_OF_SYNC to false does nothing.

This option is only available for npm and yarn projects.

Snyk docs.

How to test

I will use this branch's version of the task to test the support-doctom-components snyk integration and take it from there.
-> This solved the issue

How can we measure success?

Have we considered potential risks?

@MaelGNM MaelGNM marked this pull request as ready for review March 7, 2023 09:59
Copy link
Member

@tomrf1 tomrf1 left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Member

@akash1810 akash1810 left a comment

Choose a reason for hiding this comment

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

I do not think this is correct, and we should instead ensure the lock files are in sync. Node package managers have flags to support this in CI.

In yarn, we have yarn install --frozen-lockfile:

If you need reproducible dependencies, which is usually the case with the continuous integration systems, you should pass --frozen-lockfile flag.

In NPM, we have npm ci:

This command is similar to npm install, except it's meant to be used in automated environments such as test platforms, continuous integration, and deployment -- or any situation where you want to make sure you're doing a clean install of your dependencies.

I think we should use these flags when installing dependencies in CI, so that the build fails if the lock file is out of date, resulting in our keeping it up to date. This also results in CI becoming more deterministic, as the versions of (direct, and transitive) dependencies installed during the build will exactly match those used locally when developing a change.

@akash1810
Copy link
Member

I think this is a duplicate of #42?

@MaelGNM
Copy link
Author

MaelGNM commented Apr 4, 2023

I do not think this is correct, and we should instead ensure the lock files are in sync.

@akash1810 The issue is that the out of sync package is not an NPM package but another yarn workspace from this repo.

In support-dotcom-components we have shared, which both server and modules depend on. When Snyk tries to monitor server (and modules) it finds a reference to @sdc/shared but that's of course not in the lock file so it throws the out of sync error.

We have a project to move modules out of this repo and we could then refactor to remove shared but that won't be happening until next quarter at the earliest. In the meantime we would need this as a workaround to make sure we can monitor the repository.

akash1810 added a commit to guardian/support-dotcom-components that referenced this pull request Apr 4, 2023
Install the shared module `@sdc/shared` with a local path.
This allows it to appear in the `yarn.lock` file.

See:
  - https://docs.npmjs.com/cli/v9/configuring-npm/package-json#local-paths
  - guardian/.github#49
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