Add support for file based module in bundle #348
Merged
Conversation
This file contains hidden or 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
|
Okay I added a decent amount of tests, few lines of doc and refactored part of the code that I was not happy with. @stefanprodan it's ready for review. |
stefanprodan
approved these changes
Mar 2, 2024
There was a problem hiding this comment.
LGTM
Thanks @b4nst this is an awesome feature 🏅
PS. Going to add e2e tests in CI for local bundles in a followup PR.
This was referenced Mar 2, 2024
Merged
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.
Hey folks! This PR brings local module support to Bundles. Since it's quite a refactor, I'd really appreciate your input before I go any further. Hence why it's lacking tests, documentation and such.
Important
Heads up, this PR involves some hefty changes to the module fetcher.
While putting together the changes we discussed privately and in #268, I realized it's going to bulk up the current
Fetchermodule quite a bit. So, I've gone ahead and created a newfetcherpackage within theengine. This should make things more manageable and set the stage for adding new fetcher types down the road (like an HTTPS fetcher). What do you think, @stefanprodan?At the moment, the
Localfetcher only handles directories (thinktimoni apply). I didn't want to cram too much into one PR, so I've held off on adding.tar.gzfile support for now. I'll tackle that in a separate PR.I've given it a test spin with a modified version of podinfo, which I've linked here for you to try out (it uses the local redis). If this approach aligns with our goals, it could be a winner.
Before I mark this PR as ready, I'll beef up the testing (especially in the new fetcher package), flesh out the documentation, and tidy up the
BundleBuilderfor easier maintenance. I'd love to hear your thoughts on these next steps!