feat(yarnv1): read package.json & yarnlock #705

ben-alkov · 2024-10-23T13:53:18Z

Closes GH Issue #627

What

Ensure that 'package.json' and 'yarn.lock' files exist
Read from 'package.json' and 'yarn.lock' files

Reject packages as unsupported if PnP (Plug n' Play) is being used

i.e. any of
- installConfig.pnp: true in 'package.json'
- file(s) with glob name '*.pnp.cjs' exist
- any expanded 'node_modules' directory exists

How

add Project class
add class hierarchy for config files (CommonConfigFile ABC)
implement _check_pnp_installs() in 'main.py'
add 'pyarn' to requirements (for parsing 'yarn.lock')

N.B. RE: mypy

I had to add type: ignore for the 'pyarn' import, as 'pyarn' is untyped
Removed altogether ~~main.py::_resolve_yarn_project() has a temporary type: ignore for its return type, as it has no return yet (coming later from a different issue)~~

Maintainers will complete the following section

Commit messages are descriptive enough
Code coverage from testing does not decrease and new code is covered
Docs updated (if applicable)
Docs links in the code are still valid (if docs were updated)

Note: if the contribution is external (not from an organization member), the CI
pipeline will not run automatically. After verifying that the CI is safe to run:

approve GitHub Actions workflows by clicking a button
approve the Red Hat Trusted App Pipeline container build by commenting /ok-to-test
(as is the standard for Pipelines as Code)

cachi2/core/package_managers/yarn_classic/project.py

+        # if installConfig.pnp:
+        #     if self.yarn_cache.path.exists() and self.yarn_cache.path.is_dir():
+        #         # in this case the cache folder will be populated with downloaded ZIP dependencies
+        #         return any(file.suffix == ".zip" for file in self.yarn_cache.path.iterdir())


cachi2/core/package_managers/yarn_classic/project.py

ben-alkov · 2024-10-23T14:00:52Z

yarn.cache will be removed; handling yarnrc will be removed to a separate PR

a-ovchinnikov

A few minor suggestions and some questions.

cachi2/core/package_managers/yarn_classic/project.py

cachi2/core/package_managers/yarn_classic/main.py

ben-alkov · 2024-10-23T20:15:27Z

Some fixes and changes mentioned by Alexey are in place; removed YarnRC and yarn.cache

cachi2/core/package_managers/yarn_classic/main.py

+def _verify_repository(project: Project) -> None:
+    # _check_for_pnp(project)
+    # _check_lockfile(project)
+    pass


ben-alkov · 2024-10-24T21:16:40Z

Implemented _check_pnp_installs, is_pnp_install; removed all write() methods; added basic unit tests

cachi2/core/package_managers/yarn_classic/project.py

taylormadore · 2024-10-25T17:16:07Z

cachi2/core/package_managers/yarn_classic/project.py

+        return "package_json"
+
+    @property
+    def package_manager(self) -> Optional[str]:


At least for the initial implementation, I don't think we'll need to get/set the packageManager field in package.json

taylormadore · 2024-10-25T17:19:52Z

cachi2/core/package_managers/yarn_classic/main.py

+def _check_pnp_installs(project: Project) -> None:
+    if project.is_pnp_install:
+        raise PackageRejected(
+            reason=("Yarn zero install detected, PnP zero installs are unsupported by cachi2"),


I think "zero-installs" are a term/feature that only really exists in Yarn v2+, so we might want to avoid using it here for Yarn 1.x. Same with the solution text mention of .yarn/cache

Gah. Copy-pasta.

ben-alkov · 2024-10-25T21:09:45Z

We've got 80% coverage of new code.

I figure somewhere between 2-5 more unit tests should cover everything, and they'll all be pretty straightforward.

cachi2/core/package_managers/yarn_classic/project.py

cachi2/core/package_managers/yarn_classic/main.py

ben-alkov · 2024-10-30T15:59:44Z

Not ready to merge, as I need to increase coverage, but I'm confident in the implementation.

ben-alkov · 2024-10-31T20:41:24Z

/retest

slimreaper35 · 2024-11-05T13:27:11Z

cachi2/core/package_managers/yarn_classic/main.py

+def _resolve_yarn_project(project: Project, output_dir: RootedPath) -> list[Component]:  # type: ignore
+    # Temporary type ignore
+    """Process a request for a single yarn source directory.
+
+    :param project: the directory to be processed.
+    :param output_dir: the directory where the prefetched dependencies will be placed.
+    :raises PackageManagerError: if fetching dependencies fails
+    """
+    log.info(f"Fetching the yarn-classic dependencies at the subpath {project.source_dir}")
+
+    _verify_repository(project)
+
+    # Placeholders for implementations in other PRs
+    # _set_yarnrc_configuration(project, output_dir)
+    # packages = resolve_packages(project.source_dir)
+    # _fetch_dependencies(project.source_dir)
+
+    # return create_components(packages, project, output_dir)


This function is not used anywhere

slimreaper35 · 2024-11-05T13:34:13Z

cachi2/core/package_managers/yarn_classic/project.py

+    @property
+    def config_kind(self) -> ConfigKind:
+        """Return kind of this ConfigFile."""
+        return "yarnlock"


IMO, this property should be dropped and replaced with isinstance(<class-name>) somewhere else in the code.

The use case here is being able to use the pattern what_are_you = foo.config_kind rather than being forced to use

if isinstance(foo): what_are_you = i_am_foo elif isinstance(bar): what_are_you = i_am_bar elif isinstance(baz): what_are_you = i_am_baz

every time.

If we're truly switching a different code block for each and all classes, then yeah, the if-tree is necessary.

What would be the real use case here ? __CommonConfigFile is the abstract class so you can't instantiate it anyway.
kind = PackageJson.config_kind == "package_json" this feels awkward to me.

EDIT: So my point about isinstance is not valid, ConfigKind is just Literal.

But still don't understand the use case of such property.

EDIT 2: I understand, but using the class as type insted of literal is much clearer IMO.

slimreaper35 · 2024-11-05T13:39:05Z

cachi2/core/package_managers/yarn_classic/project.py

+        return cls(path, package_json_data)
+
+
+class YarnLock(_CommonConfigFile):


I don't understand the inheritance here, data and path attributes are not used anywhere inside YarnLock class.

I would say package.json and yarn.lock are quite different from each other.

They will be eventually. Also, they are siblings of the yarnrc class which will be implemented later

cachi2/core/package_managers/yarn_classic/project.py

slimreaper35 · 2024-11-05T13:47:37Z

cachi2/core/package_managers/yarn_classic/project.py

+        if not yarn_lockfile:
+            raise PackageRejected(
+                reason="The yarn.lock file must not be empty",
+                solution=("Please verify the content of the file."),


( and ) are not necessary here

Refactor fallout :rolleyes:

cachi2/core/package_managers/yarn_classic/project.py

slimreaper35 · 2024-11-05T13:55:35Z

As I often get these recommendations, you could split the changes into more commits. For example

implement yarn.lock
implement package.json
(package.json has nothing to do with pyarn being added to requirements.txt)

taylormadore · 2024-11-04T21:20:33Z

cachi2/core/package_managers/yarn_classic/main.py

+    return None
+
+
+def _resolve_yarn_project(project: Project, output_dir: RootedPath) -> list[Component]:  # type: ignore


I wonder if we should just drop this function in this PR and I'll introduce it in mine. That way the unit tests for detecting pnp installs could just test _verify_repository rather than (what will be) the more expansive _resolve_yarn_project

taylormadore · 2024-11-04T22:58:16Z

tests/unit/package_managers/yarn_classic/test_project.py

+        ),
+    ],
+)
+def test_pnp_installs_detection(


nit: It might make this simpler to make the error case a separate test

This is applicable to all test cases that have branching in them.

taylormadore · 2024-11-05T13:35:50Z

requirements.txt

+    --hash=sha256:fc2db02409338bf36590aa985a461b2c96fce91f8e7e0f14c50c5fcc4f229016 \
+    --hash=sha256:ffcad6c564fe6b9b8916c1aefbb37a362deebf9394bd2974e9d84232e3e08504
+    # via yarl
+pyarn==0.2.0 \


pyarn is pinned to different versions in requirements.txt and requirements-extras.txt

taylormadore · 2024-11-05T13:55:03Z

cachi2/core/package_managers/yarn_classic/project.py

+            )
+        except ValueError as e:
+            raise PackageRejected(
+                reason=(f"Can't parse the {path.subpath_from_root} file.\n" f"{e}"),


The output here could potentially be lengthy, so I think we definitely want the exceptions chained but without repeating the output in the new PackageRejected exception we are raising

cachi2/core/package_managers/yarn_classic/main.py

a-ovchinnikov · 2024-11-05T17:37:24Z

tests/unit/package_managers/yarn_classic/test_project.py

+        ),
+    ],
+)
+def test_pnp_installs_detection(


This is applicable to all test cases that have branching in them.

cachi2/core/package_managers/yarn_classic/project.py

ben-alkov · 2024-11-11T19:44:15Z

As I often get these recommendations, you could split the changes into more commits. For example
* implement yarn.lock

* implement package.json
  (package.json has nothing to do with `pyarn`  being added to requirements.txt)

Fixed

cachi2/core/package_managers/yarn_classic/project.py

a-ovchinnikov

99% LGTM (with a couple small and trivial fixes).

cachi2/core/package_managers/yarn_classic/project.py

## What - Ensure that 'package.json' file exists - Read from 'package.json' ## How - introduce ABC `_CommonConfigFile` dataclass to represent config files - implement `PackageJson` CommonConfigFile subclass Signed-off-by: Ben Alkov <[email protected]>

## What - a class representing a yarn project - can create Projects from a source dir - can check for (disallowed) PnP installs ## How - implement `Project` as a frozen dataclass Signed-off-by: Ben Alkov <[email protected]>

## What - Ensure that 'yarn.lock' file exists - Read from 'yarn.lock' ## How - add 'pyarn' to dependencies (to parse 'yarn.lock' file) - implement `YarnLock` CommonConfigFile subclass Signed-off-by: Ben Alkov <[email protected]>

github-advanced-security bot found potential problems Oct 23, 2024

View reviewed changes

ben-alkov marked this pull request as draft October 23, 2024 13:54

github-advanced-security bot found potential problems Oct 23, 2024

View reviewed changes

cachi2/core/package_managers/yarn_classic/project.py Fixed Show fixed Hide fixed

a-ovchinnikov reviewed Oct 23, 2024

View reviewed changes

github-advanced-security bot found potential problems Oct 23, 2024

View reviewed changes

cachi2/core/package_managers/yarn_classic/main.py Fixed Show fixed Hide fixed

github-advanced-security bot found potential problems Oct 23, 2024

View reviewed changes

cachi2/core/package_managers/yarn_classic/main.py Outdated

def _verify_repository(project: Project) -> None:

# _check_for_pnp(project)

# _check_lockfile(project)

pass

Check warning

Code scanning / CodeQL

Unnecessary pass

Unnecessary 'pass' statement.

taylormadore mentioned this pull request Oct 24, 2024

Yarn 1.x: Parse packages from yarn.lock #693

Open

4 tasks

ben-alkov force-pushed the feat-yarnv1-read-pkg-json-yarn-lock branch from 5d2425d to 63b7946 Compare October 24, 2024 21:14

taylormadore reviewed Oct 25, 2024

View reviewed changes

ben-alkov force-pushed the feat-yarnv1-read-pkg-json-yarn-lock branch from 63b7946 to 903bd5b Compare October 25, 2024 21:08

a-ovchinnikov reviewed Oct 28, 2024

View reviewed changes

cachi2/core/package_managers/yarn_classic/project.py Outdated Show resolved Hide resolved

cachi2/core/package_managers/yarn_classic/main.py Outdated Show resolved Hide resolved

ben-alkov force-pushed the feat-yarnv1-read-pkg-json-yarn-lock branch from ea7eeb4 to 9321a9f Compare October 30, 2024 15:53

ben-alkov marked this pull request as ready for review October 30, 2024 15:59

ben-alkov force-pushed the feat-yarnv1-read-pkg-json-yarn-lock branch 2 times, most recently from b06b279 to 0e585af Compare October 31, 2024 19:18

ben-alkov requested review from a-ovchinnikov and taylormadore October 31, 2024 20:23

slimreaper35 reviewed Nov 5, 2024

View reviewed changes

taylormadore reviewed Nov 5, 2024

View reviewed changes

a-ovchinnikov reviewed Nov 5, 2024

View reviewed changes

ben-alkov force-pushed the feat-yarnv1-read-pkg-json-yarn-lock branch from 0e585af to 1ffda55 Compare November 11, 2024 19:42

ben-alkov requested a review from slimreaper35 November 11, 2024 22:15

ben-alkov requested review from a-ovchinnikov and taylormadore November 11, 2024 22:15

slimreaper35 reviewed Nov 12, 2024

View reviewed changes

cachi2/core/package_managers/yarn_classic/project.py Outdated Show resolved Hide resolved

ben-alkov force-pushed the feat-yarnv1-read-pkg-json-yarn-lock branch from 1ffda55 to c0892c3 Compare November 12, 2024 14:04

a-ovchinnikov reviewed Nov 12, 2024

View reviewed changes

cachi2/core/package_managers/yarn_classic/project.py Outdated Show resolved Hide resolved

cachi2/core/package_managers/yarn_classic/project.py Outdated Show resolved Hide resolved

cachi2/core/package_managers/yarn_classic/project.py Show resolved Hide resolved

ben-alkov added 3 commits November 12, 2024 11:48

feat(yarn_classic): implement 'Project' class

feaa970

## What - a class representing a yarn project - can create Projects from a source dir - can check for (disallowed) PnP installs ## How - implement `Project` as a frozen dataclass Signed-off-by: Ben Alkov <[email protected]>

feat(yarn_classic): implement 'yarn.lock' support

8a2e7d3

## What - Ensure that 'yarn.lock' file exists - Read from 'yarn.lock' ## How - add 'pyarn' to dependencies (to parse 'yarn.lock' file) - implement `YarnLock` CommonConfigFile subclass Signed-off-by: Ben Alkov <[email protected]>

ben-alkov force-pushed the feat-yarnv1-read-pkg-json-yarn-lock branch from c0892c3 to 8a2e7d3 Compare November 12, 2024 16:48

a-ovchinnikov approved these changes Nov 12, 2024

View reviewed changes

ben-alkov requested a review from slimreaper35 November 12, 2024 20:35

slimreaper35 approved these changes Nov 13, 2024

View reviewed changes

ben-alkov added this pull request to the merge queue Nov 13, 2024

Merged via the queue into containerbuildsystem:main with commit 58faa67 Nov 13, 2024
15 of 16 checks passed

		return cls(path, package_json_data)


		class YarnLock(_CommonConfigFile):

		return None


		def _resolve_yarn_project(project: Project, output_dir: RootedPath) -> list[Component]: # type: ignore

feat(yarnv1): read package.json & yarnlock #705

feat(yarnv1): read package.json & yarnlock #705

Conversation

ben-alkov commented Oct 23, 2024 • edited Loading

What

How

N.B. RE: mypy

Maintainers will complete the following section

ben-alkov commented Oct 23, 2024

a-ovchinnikov left a comment

Choose a reason for hiding this comment

ben-alkov commented Oct 23, 2024

ben-alkov commented Oct 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ben-alkov commented Oct 25, 2024

ben-alkov commented Oct 30, 2024

ben-alkov commented Oct 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slimreaper35 Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ben-alkov Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slimreaper35 commented Nov 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ben-alkov commented Nov 11, 2024

a-ovchinnikov left a comment

Choose a reason for hiding this comment

ben-alkov commented Oct 23, 2024 •

edited

Loading

slimreaper35 Nov 7, 2024 •

edited

Loading

ben-alkov Nov 7, 2024 •

edited

Loading