bundler: Provide the build configuration #654
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
eskultety
reviewed
Sep 18, 2024
a-ovchinnikov
force-pushed
the
bundler_provide_build_config
branch
from
f1d896d
to
8137e5d
Compare
a-ovchinnikov
force-pushed
the
bundler_provide_build_config
branch
from
8137e5d
to
dded3d3
Compare
a-ovchinnikov
force-pushed
the
bundler_provide_build_config
branch
from
dded3d3
to
bc06622
Compare
a-ovchinnikov
requested review from
brunoapimentel and
slimreaper35
and removed request for
brunoapimentel
a-ovchinnikov
force-pushed
the
bundler_provide_build_config
branch
from
bc06622
to
c7666db
Compare
eskultety
reviewed
Sep 30, 2024
There was a problem hiding this comment.
Since unlike Yarn v1 none of this bundler work is tracked as upstream issues, I'm missing the part where the config is actually
Edit: Since I eventually provided all comments inline, please ignore ^this snippet I forgot to delete before submitting the review.
a-ovchinnikov
force-pushed
the
bundler_provide_build_config
branch
2 times, most recently
from
c7764b3
to
b1e2183
Compare
slimreaper35
reviewed
Oct 1, 2024
Comment on lines
+56
to
+83
| pytest.param( | ||
| utils.TestParameters( | ||
| repo="https://github.com/cachito-testing/cachi2-bundler.git", | ||
| ref="well_formed_ruby_all_features", | ||
| packages=({"path": ".", "type": "bundler"},), | ||
| flags=["--dev-package-managers"], | ||
| check_output=False, | ||
| check_deps_checksums=False, | ||
| check_vendor_checksums=False, | ||
| expected_exit_code=0, | ||
| expected_output="", | ||
| ), | ||
| id="bundler_everything_present", | ||
| ), | ||
| pytest.param( | ||
| utils.TestParameters( | ||
| repo="https://github.com/cachito-testing/cachi2-bundler.git", | ||
| ref="well_formed_ruby_without_gemspec", | ||
| packages=({"path": ".", "type": "bundler"},), | ||
| flags=["--dev-package-managers"], | ||
| check_output=False, | ||
| check_deps_checksums=False, | ||
| check_vendor_checksums=False, | ||
| expected_exit_code=0, | ||
| expected_output="", | ||
| ), | ||
| id="bundler_everything_present_except_gemspec", | ||
| ), |
There was a problem hiding this comment.
Is it worth adding a scenario with multiple packages in the same repo ?
There was a problem hiding this comment.
Probably not, at least not with our current ITs structure. This scenario would essentially exercise the same code that is exercised in existing tests, but in a loop.
There was a problem hiding this comment.
One last thing. We don't have any e2e tests and I believe we could make one of those tests above (that are almost identical) to do full pre-fetch and build an image.
There was a problem hiding this comment.
Good point, I'll add a e2e test in a separate commit to this PR (after it passes locally).
eskultety
reviewed
Oct 1, 2024
In order for Bundler to build from the content prefetched by Cachi2, we need to set a few configuration parameters. There are several ways of doing this, creating an alternative config and pointing bundler to it via an environment variable was chosen as the least of all evils. The resulting config file will override all settings defined in all configs committed to all repositories within the package. An attempt is made to remediate the issue by cloning top-level config first and adding necessary values to it, but this could fail on a complex package which contains sub-packages and nested local configs. Such cases should be very rare and this risk is deemed acceptable. Signed-off-by: Alexey Ovchinnikov <[email protected]>
a-ovchinnikov
force-pushed
the
bundler_provide_build_config
branch
from
b1e2183
to
b2e7d26
Compare
eskultety
approved these changes
Oct 2, 2024
slimreaper35
approved these changes
Oct 3, 2024
Merged
via the queue into
containerbuildsystem:main
with commit Oct 3, 2024
93915e7
16 checks passed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In order for Bundler to build from the content prefetched by Cachi2, we need to set a few configuration parameters. There are several ways of doing this, but the most robust one is to place a local configuration file into processed repository (see docs/design/bundler.md for details).
This commit introduces a fetch step that presets configuraion parameters.
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)