cachi2 yarn: prefetch dependencies for regular workflow #342

slimreaper35 · 2023-10-06T13:22:40Z

Maintainers will complete the following section

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

cachi2/core/package_managers/yarn/main.py

brunoapimentel

I also think we should unit test the _fetch_dependencies function, even though we'll likely end up with a very basic test that essentially guarantees that the function parameters are being handled correctly.

cachi2/core/package_managers/yarn/main.py

brunoapimentel · 2023-10-09T22:23:11Z

Also, since this seems to be more straightforward than I imagined, wdyt about also checking the cache for the zero-installs workflow in this PR (STONEBLD-1789)? It also boils down to calling yarn install.
In hindsight, this was a bad idea. It's probably better to have a small PR for it later, as planned.

cachi2/core/package_managers/yarn/main.py

cachi2/core/errors.py

cachi2/core/package_managers/yarn/main.py

brunoapimentel · 2023-10-10T14:56:28Z

We should also make the run_yarn_cmd function fully functional. It is currently still returning a NotImplemented.

cachi2/core/package_managers/yarn/main.py

cachi2/core/errors.py

cachi2/core/package_managers/yarn/main.py

chmeliik · 2023-10-11T09:09:29Z

cachi2/core/package_managers/yarn/main.py

-    # the output_dir is where the globalFolder must be set to
-    # the yarn commands can be called by using the core.utils.run_cmd function
-    pass
+    cachi2_output = output_dir.join_within_root("deps", "yarnberry")


Any opinions on the file path? It's not technically public API since the output directory structure should be considered opaque, but users will most likely see these paths somewhere, e.g. source containers once RHTAP implements them

If Yarn Berry is the version of Yarn for us, we should probably just call it deps/yarn

If Yarn Berry is the version of Yarn for us, we should probably just call it deps/yarn

Without having literally any knowledge of Yarn, if there are multiple versions/flavours of Yarn packaging you already do support or may support in the future deps/yarn will bite us in the back for sure. We should either refer to it by the official name (some lowercase name variant) or yarn + version if those are a thing by which it could be determined whether it's Yarn berry or something else.

As for users, if it's not part of the official public API, it doesn't matter at all as the paths are bound to change in that case, so whatever they do on them/expect from them is irrelevant in that case, so we're free to alter it to our liking. Of course the question then is whether we should make these deps paths officially part of the API so we guarantee them - which we should do if we refer to them in any shape or form in the docs... just my 2c

There's yarn v1: https://classic.yarnpkg.com/en/docs
And yarn v2+: https://yarnpkg.com/

The official name for both is Yarn, but they might as well be two entirely different package managers. V1 is sometimes referred to as Yarn Classic, v2+ as Yarn Berry.

Yarn v1 is supported in Cachito but not in Cachi2, v2+ is not supported in Cachito and will be in Cachi2. There is some hope that we wouldn't have to implement v1 in Cachi2, but I'm not among the optimists 😄

In Cachito Yarn v1 deps went to deps/yarn, but I don't think we need to keep backwards compatibility with those, they never mattered to any system or user AFAIK. (At least the deps/yarn part of the filepath didn't, the files themselves did)

Of course the question then is whether we should make these deps paths officially part of the API so we guarantee them - which we should do if we refer to them in any shape or form in the docs... just my 2c

True, we should probably consider changes in the path as breaking changes

There's a separate matter of what do we call the package manager in the cachi2 input (now this very much is public API)

cachi2 fetch-deps yarn?

cachi2 fetch-deps yarnberry?

But maybe cachi2 should be smart, accept only yarn and auto-detect which yarn it is. We had a story in the backlog to figure this out, but deferred it because yarn v1 support may not be needed.

TLDR: deps/yarn is fine by me 😄

Did not expect such a long discussion about the file path 😄

Should I rename YarnBerryError to YarnError to keep it consistent?

Should I rename YarnBerryError to YarnError to keep it consistent?

To me, that makes sense 👍

Should I rename YarnBerryError to YarnError to keep it consistent?

Yeah, I think dropping Berry from the name makes sense. The error message is probably generic enough to be usable for all flavours of yarn anyway. Maybe call it YarnCommandError though, YarnError seems too generic

cachi2/core/package_managers/yarn/main.py

chmeliik

LGTM apart from the output directory name 👍

taylormadore

LGTM 👍

chmeliik · 2023-10-13T08:19:05Z

For future reference, here's what I imagine a commit message for a change like this could look like

Use 'yarn install --mode=skip-build' to download dependencies for
projects that do not use zero-installs. That is, projects which do not
contain a cacheFolder directory (default .yarn/cache/) with the
dependencies already downloaded. Such projects will be handled
separately.

While running the command, set the YARN_GLOBAL_FOLDER environment
variable to '$cachi2_output_dir/deps/yarn' to make sure yarn downloads
the dependencies into this directory.

The --mode=skip-build option is required to avoid executing the
preinstall, install and postinstall scripts if the project has any.
Such scripts are arbitrary code, which we should not execute.

Though some parts of this probably wouldn't be necessary if they were code comments

The current commit message also says YarnBerryError rather than YarnCommandError

* with new custom YarnCommandError * covered with unit tests JIRA: STONEBLD-1788 Signed-off-by: Michal Šoltis <[email protected]>

slimreaper35 · 2023-10-13T09:43:03Z

Maybe we could start using any commit message template or pre-commit or something that is automated.

I scrolled down commit history a little bit and I rarely saw such a verbose commit message (except for your commits 😄 ). Mostly it was a JIRA ticket number and even for larger PRs than this one.

WDYT ?

eskultety · 2023-10-13T09:43:24Z

For future reference, here's what I imagine a commit message for a change like this could look like

To take effect, this needs to be defined in some kind of contributor guidelines which we currently don't have, but yeah, totally in agreement.

eskultety · 2023-10-13T09:49:11Z

Maybe we could start using any commit message template or pre-commit or something that is automated.

You'd have to use AI to provide any meaningful gist for the commit subject, we're not there yet :). Writing good commit messages is a skill worth having rather than trying to automate it. Think of it as a way of explaining your changes to a real person.

I scrolled down commit history a little bit and I rarely saw such a verbose commit message (except for your commits 😄 ). Mostly it was a JIRA ticket number and even for larger PRs than this one.

The fact that the commit history is scarce in detailed information in this case must not serve as a precedent preventing adoption of an improvement in the future. Projects and communities evolve gradually :).

slimreaper35 · 2023-10-13T09:57:57Z

I am sorry, I meant that the check of the commit message would automated at least at some form. I think it would be fine for consistency of all commits. Since check in the PR template is quite subjective.

Second point, I did not want to sound it like an excuse, but I feel like I am the only one who is blamed for commit messages.

chmeliik · 2023-10-13T10:46:51Z

I am sorry, I meant that the check of the commit message would automated at least at some form. I think it would be fine for consistency of all commits. Since check in the PR template is quite subjective.

Second point, I did not want to sound it like an excuse, but I feel like I am the only one who is blamed for commit messages.

I don't think verbosity is always needed, but the message does need to describe what the change does and explain things that are not obvious from the code or comments. For more complex changes, the message should help the reviewer understand what they're getting into.

It absolutely is subjective as you said, and we're not always good at enforcing the checklist. Sorry if it looks like you're the only one getting called out for non-descriptive commits 😞 (I'm pretty sure others do as well)

Using a commit message template could be a good idea 👍

chmeliik

LGTM 👍

slimreaper35 · 2023-10-13T11:24:02Z

One more thing...In this PR I should split the one commit into probably two (implement YarnCommandError and implement fetching), but usually I make changes along the discussion (which is completely fine), and then I would have to rearrange commits as well, which is not always straightforward so it leads to one or more big commits where I have to explain everything. I wish I could do it better from the beginning. Thats the skill I want to have. I think it would not lead to those kind of discussions.

But anyaway, I agree that something like commit template is a good idea. Thats my final words. :drop-the-mike:

chmeliik · 2023-10-13T12:06:53Z

One more thing...In this PR I should split the one commit into probably two (implement YarnCommandError and implement fetching), but usually I make changes along the discussion (which is completely fine), and then I would have to rearrange commits as well, which is not always straightforward so it leads to one or more big commits where I have to explain everything. I wish I could do it better from the beginning. Thats the skill I want to have. I think it would not lead to those kind of discussions.

Do you use git rebase --interactive? That's what I do to re-arrange commits, insert a new commit between old ones, edit an older commit etc.

But anyaway, I agree that something like commit template is a good idea. Thats my final words. :drop-the-mike:

Can you look into that, or bring it up in a team discussion?

slimreaper35 marked this pull request as ready for review October 9, 2023 08:51

slimreaper35 requested review from brunoapimentel, taylormadore, ben-alkov, lkolacek, chmeliik and ejegrova October 9, 2023 09:16

eskultety reviewed Oct 9, 2023

View reviewed changes

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

brunoapimentel reviewed Oct 9, 2023

View reviewed changes

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

chmeliik reviewed Oct 10, 2023

View reviewed changes

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

slimreaper35 requested review from brunoapimentel, chmeliik and eskultety October 10, 2023 13:12

chmeliik reviewed Oct 10, 2023

View reviewed changes

cachi2/core/errors.py Outdated Show resolved Hide resolved

cachi2/core/errors.py Outdated Show resolved Hide resolved

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

brunoapimentel reviewed Oct 10, 2023

View reviewed changes

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

chmeliik reviewed Oct 10, 2023

View reviewed changes

cachi2/core/errors.py Outdated Show resolved Hide resolved

slimreaper35 requested review from brunoapimentel and chmeliik October 11, 2023 07:34

chmeliik reviewed Oct 11, 2023

View reviewed changes

chmeliik reviewed Oct 12, 2023

View reviewed changes

taylormadore approved these changes Oct 12, 2023

View reviewed changes

Implement fetching dependencies for yarn regular workflow

adaa6ab

* with new custom YarnCommandError * covered with unit tests JIRA: STONEBLD-1788 Signed-off-by: Michal Šoltis <[email protected]>

chmeliik approved these changes Oct 13, 2023

View reviewed changes

chmeliik added this pull request to the merge queue Oct 13, 2023

Merged via the queue into containerbuildsystem:main with commit a9e41e2 Oct 13, 2023

slimreaper35 deleted the STONEBLD-1788 branch October 13, 2023 16:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cachi2 yarn: prefetch dependencies for regular workflow #342

cachi2 yarn: prefetch dependencies for regular workflow #342

slimreaper35 commented Oct 6, 2023 •

edited by chmeliik

Loading

brunoapimentel left a comment

brunoapimentel commented Oct 9, 2023 •

edited

Loading

brunoapimentel commented Oct 10, 2023

chmeliik Oct 11, 2023

eskultety Oct 11, 2023

eskultety Oct 11, 2023

chmeliik Oct 11, 2023 •

edited

Loading

chmeliik Oct 11, 2023

chmeliik Oct 12, 2023 •

edited

Loading

chmeliik Oct 12, 2023

slimreaper35 Oct 12, 2023

taylormadore Oct 12, 2023

chmeliik Oct 12, 2023

chmeliik left a comment

taylormadore left a comment

chmeliik commented Oct 13, 2023

slimreaper35 commented Oct 13, 2023

eskultety commented Oct 13, 2023

eskultety commented Oct 13, 2023 •

edited

Loading

slimreaper35 commented Oct 13, 2023

chmeliik commented Oct 13, 2023

chmeliik left a comment

slimreaper35 commented Oct 13, 2023 •

edited

Loading

chmeliik commented Oct 13, 2023

cachi2 yarn: prefetch dependencies for regular workflow #342

cachi2 yarn: prefetch dependencies for regular workflow #342

Conversation

slimreaper35 commented Oct 6, 2023 • edited by chmeliik Loading

Maintainers will complete the following section

brunoapimentel left a comment

Choose a reason for hiding this comment

brunoapimentel commented Oct 9, 2023 • edited Loading

brunoapimentel commented Oct 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chmeliik Oct 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chmeliik Oct 12, 2023 • 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

chmeliik left a comment

Choose a reason for hiding this comment

taylormadore left a comment

Choose a reason for hiding this comment

chmeliik commented Oct 13, 2023

slimreaper35 commented Oct 13, 2023

eskultety commented Oct 13, 2023

eskultety commented Oct 13, 2023 • edited Loading

slimreaper35 commented Oct 13, 2023

chmeliik commented Oct 13, 2023

chmeliik left a comment

Choose a reason for hiding this comment

slimreaper35 commented Oct 13, 2023 • edited Loading

chmeliik commented Oct 13, 2023

slimreaper35 commented Oct 6, 2023 •

edited by chmeliik

Loading

brunoapimentel commented Oct 9, 2023 •

edited

Loading

chmeliik Oct 11, 2023 •

edited

Loading

chmeliik Oct 12, 2023 •

edited

Loading

eskultety commented Oct 13, 2023 •

edited

Loading

slimreaper35 commented Oct 13, 2023 •

edited

Loading