yarn: Automated Yarn version detector #743
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
@a-ovchinnikov; Definitely a "it's not you, it's me" thing, and something which I know there are many valid opinions about; if it were me, I'd order it like this, to make it easier to review: def _get_path_to_yarn_lock()
def _contains_yarn_version_trait()
_yarnv1_pattern
_yarnv2_pattern
contains_yarn_classic
_yarn_versions
_yarn_processors
def _yarn_selector()
def _separate_packages_by_yarn_version()
def dispatch_to_correct_fetcher() |
Fair enough, done. |
a-ovchinnikov
force-pushed
the
issue624
branch
2 times, most recently
from
ab8910d to
734b198
Compare
a-ovchinnikov
changed the title
|
Marking as ready for review to encourage reviews. Will squash the commits once we agree upon the approach. |
|
The updated approach here seems straightforward 👍 IIRC the current v1 parser does check for a v1 lockfile, but that parsing occurs later in the request processing, so I don't have an objection to doing it separately and earlier as proposed here |
a-ovchinnikov
force-pushed
the
issue624
branch
3 times, most recently
from
280b015 to
0390c22
Compare
|
I think we might need to add a config option that would disable the processing of yarn v2+ requests altogether. I'm thinking specifically of cachi2-in-OSBS here, where we probably want to restrict input to yarn v1. I like the idea of a config option over a flag, since I don't believe we will allow cachi2 config access to users in OSBS. CC: @MartinBasti |
a-ovchinnikov
force-pushed
the
issue624
branch
from
0390c22 to
27ff546
Compare
Done! |
a-ovchinnikov
force-pushed
the
issue624
branch
from
27ff546 to
ffa602a
Compare
| class MissingLockfile(PackageRejected): | ||
| """Indicate that a lock file is missing.""" | ||
|
|
||
| def __init__(self) -> None: | ||
| """Initialize a Missing Lockfile error.""" | ||
| reason = "Yarn lockfile 'yarn.lock' missing, refusing to continue" | ||
| solution = "Make sure your repository has a Yarn lockfile (i.e. yarn.lock) checked in" | ||
| super().__init__(reason, solution=solution) | ||
|
|
||
|
|
||
| class NotV1Lockfile(PackageRejected): | ||
| """Indicate that a lockfile is of wrong tyrpoe.""" | ||
|
|
||
| def __init__(self, package_path: Any) -> None: | ||
| """Initialize a Missing Lockfile error.""" | ||
| reason = f"{package_path} not a Yarn v1" | ||
| super().__init__(reason, solution=None) |
There was a problem hiding this comment.
Recently, I got a comment from Erik, that all custom cachi2 exceptions should be inside cachi2.core.errors.py. But that's just a nitpick.
There was a problem hiding this comment.
Correction - I meant any publicly exposed top-level exceptions like PathOutsideRoot, PackageRejected, etc. ...custom exceptions which are module-scoped are fine to keep in the given module where it makes the most sense for them. That said, these are module-scoped exceptions and so we should start their name with an underscore. Additionally, these should be re-raised in the fetch-yarn-... meta entrypoint as a PackageRejected exception IMO as that's the public-facing one.
There was a problem hiding this comment.
These are not module-scoped, but they are also not cachi2-scoped: they are raised here to send a message to meta-PM. They do not need to be re-wrapped because as we have agreed earlier everything that is not v1 is v2+ (and exceptions from v2+ propagate freely). Furthermore, any other exception will propagate to the user, so if something happens to conform to v1 and not trigger any of these, and also is a broken v1 then a user would know.
a-ovchinnikov
force-pushed
the
issue624
branch
from
ffa602a to
5a89096
Compare
| from cachi2.core.utils import merge_outputs | ||
|
|
||
|
|
||
| def fetch_yarn_source(request: Request) -> RequestOutput: |
There was a problem hiding this comment.
We need a unit test for this function, as the coverage is very low on this module.
In order to simplify user interface some logic is added to automatically distinguish between supported Yarn versions. Addresses: containerbuildsystem#624 Signed-off-by: Alexey Ovchinnikov <[email protected]>
a-ovchinnikov
force-pushed
the
issue624
branch
from
5a89096 to
ceb2583
Compare
|
One detail I missed before is that part of 78d0eeb (specifically the part allowing yarn-classic as an input to the CLI) still needs to be reverted Otherwise LGTM 💯 |
With the introduction of automated yarn packages dispatch it does not make sense to keep user-visible yarn-classic references. This commit removes all public mentions of it. Signed-off-by: Alexey Ovchinnikov <[email protected]>
Done. |
6d2a106
In order to simplify user interface some logic is added to automatically distinguish between supported Yarn versions.
Maintainers will complete the following section
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:
/ok-to-test(as is the standard for Pipelines as Code)