Closed eskultety closed 3 months ago
Note the unit test is extremely basic and I'd like to rework it so that we test the outer async_download_files
function, but hey, it's Friday, so for the initial round of reviews it should be fine :) . If you want to test this with e.g. the tech-preview rpms, this is a good dummy file: https://dl.fedoraproject.org/pub/fedora/linux/releases/40/Everything/source/tree/Packages/g/google-noto-fonts-20240301-2.fc40.src.rpm
Since v1:
Note the unit test is extremely basic and I'd like to rework it so that we test the outer async_download_files function
Eventually, I kept the original basic unit test as it seems more sufficient than originally after having reworked the PR for a v2, so this PR can be reviewed in its completeness.
Thank you!
Going a little further out of the scope of this PR: we'll also need to introduce a way to define a custom config file in build-definitions. Which makes me wonder that the current UX for this is less than ideal.
If anything, doing a cachi2 fetch-deps --help
will not show that the --config-file
option is avaible, because it is tied to the upper cachi2
command only (cachi2 --help
will show it). I think that Cachi2 respecting a custom config file placed in the target --sources
folder could bring more flexibility to the end user.
Anyways, some random thoughts, this PR LGTM.
Going a little further out of the scope of this PR: we'll also need to introduce a way to define a custom config file in build-definitions. Which makes me wonder that the current UX for this is less than ideal.
If anything, doing a
cachi2 fetch-deps --help
will not show that the--config-file
option is avaible, because it is tied to the uppercachi2
command only (cachi2 --help
will show it). I think that Cachi2 respecting a custom config file placed in the target--sources
folder could bring more flexibility to the end user.Anyways, some random thoughts, this PR LGTM.
Yes, this is completely out of scope for this PR. Note that default config file paths should always be either $HOME/.<filename>
or $HOME/.config/<project>/filename
, so with consistency with other established tools in mind if the path differs, it should be used with a flag, i.e. --config-file
.
FWIW I don't think config files are flexible enough for the use case you describe, so I'm instead intending to add support for environment variables (matching the config options) to take precedence over config files which should give us all the flexibility we need in purely containerized environments.
Note that default config file paths should always be either
$HOME/.<filename>
or$HOME/.config/<project>/filename
, so with consistency with other established tools in mind if the path differs, it should be used with a flag, i.e.--config-file
.
Syft, for example, allows the user to define configuration per folder, and I think it is pretty useful since it may vary from project to project.
so I'm instead intending to add support for environment variables
This is probably good enough, especially since we don't have a lot of config options. If the number of config options increases, this may be harder for users to manage.
Note that default config file paths should always be either
$HOME/.<filename>
or$HOME/.config/<project>/filename
, so with consistency with other established tools in mind if the path differs, it should be used with a flag, i.e.--config-file
.Syft, for example, allows the user to define configuration per folder, and I think it is pretty useful since it may vary from project to project.
What would be the benefit of injecting the config file to CWD (i.e. the source repo dir) instead of an arbitrary dir via volumes in container world and even more specifically in tekton definitions? So far the only difference I can see is the usage of --config-file
(which you wouldn't need to use in the former case). Is there anything else? Apart from the obvious case (which nobody wants) where users checked in the config directly to their repos... Unless there's something I'm not seeing ATM to me, having/not having to specify --config-file
option doesn't seem like a massive improvement or a deal breaker from convenience or usage POV really, however...
so I'm instead intending to add support for environment variables
...this is IMO gives users a clear benefit of not having to create and inject any files which is a big plus in containerized environments. The clear benefits I can see with environment variables:
with the last one for me being the deal breaker, IOW be appreciate package managers supporting env variables (e.g. Yarn which matches the env var name with the corresponding setting in the config file) which makes our job that much easier since we don't have to craft solution tailored too much to a given situation.
This is probably good enough, especially since we don't have a lot of config options. If the number of config options increases, this may be harder for users to manage.
What's the difference between having to set 10 variables vs having to dump 10 options to a config file created on an ad-hoc basis and injected to the resulting container environment as a volume? I'm not sure I follow your point about scaling wrt/ the number of env variables available (provided these are clearly documented).
Note that default config file paths should always be either
$HOME/.<filename>
or$HOME/.config/<project>/filename
, so with consistency with other established tools in mind if the path differs, it should be used with a flag, i.e.--config-file
.Syft, for example, allows the user to define configuration per folder, and I think it is pretty useful since it may vary from project to project.
What would be the benefit of injecting the config file to CWD (i.e. the source repo dir) instead of an arbitrary dir via volumes in container world and even more specifically in tekton definitions? So far the only difference I can see is the usage of
--config-file
(which you wouldn't need to use in the former case). Is there anything else? Apart from the obvious case (which nobody wants) where users checked in the config directly to their repos... Unless there's something I'm not seeing ATM to me, having/not having to specify--config-file
option doesn't seem like a massive improvement or a deal breaker from convenience or usage POV really, however...
On a second thought, benefits aside or the actual impact, it should be cheap for us to add support for both default directory config location as well as CWD at the same time, so we can definitely track it as an improvement, @brunoapimentel I went ahead and created: #573 and #574
We only honour the request timeout from user configuration file with synchronous downloads but not with asyncio/aiohttp. Apply the user config timeout if it's larger than the default aiohttp one [1].
[1] https://docs.aiohttp.org/en/stable/client_reference.html#aiohttp.ClientSession
Resolves: https://github.com/containerbuildsystem/cachi2/issues/571
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)