Add extension to allow bazelmod style handling of dependencies #124
Conversation
mering
left a comment
mering
left a comment
There was a problem hiding this comment.
Thanks for taking the time to proposing this upstream!
helm/extensions.bzl
Outdated
| tag_classes = {"options": options}, | ||
| tag_classes = { | ||
| "options": options, | ||
| "import_repository": tag_class(attrs = { |
There was a problem hiding this comment.
Maybe call this chart such that helm.chart(...) imports a chart. As this is used in MODULE.bazel I can't imagine anything other than importing from a repository to make sense. All other ways to import charts are probably specified via BUILD files.
There was a problem hiding this comment.
NAK
I value the consistency with the documented functions in the Readme.md over isolated potentially better name.
In context, it's also obvious that this is rather: "import from repository" than "import a repository", as there's also a "helm_import" function.
There was a problem hiding this comment.
I would be happy to rename things if it makes more sense to the larger community. I'm only really familiar with Bazel terminology and loosely familiar with Helm's so whatever names I chose would have skewed more Bazel-like. I do think import_repository is a bit lengthy so maybe import if not chart but I also think whatever the name, docs should describe clearly what this does so people from either camp can figure it out easily.
Ongy
force-pushed
the
add-import-repository-extension
branch
from
0572050 to
eebaf6d
Compare
Ongy
force-pushed
the
add-import-repository-extension
branch
from
0124e8d to
988121b
Compare
|
@abrisco can you take a look at the naming discussion, and do another pass over the code? thx. |
| # And the _register_toolchains needs to be called *after* iteration of all modules. | ||
| for module in ctx.modules: | ||
| for repository in module.tags.import_repository: | ||
| if not module.is_root: |
There was a problem hiding this comment.
Why can't repositories be instantiated if it's not the root?
There was a problem hiding this comment.
They can but they shouldn't for now since this would require more elaborate de-duplication based on repository URL. This approach is commonly used in module extensions to facilitate future improvements. This way, one can migrate a breaking change (for example generating the name based on repository and/or chart) completely in the root module while otherwise a change in the interface would require updating all transitive dependencies first.
There was a problem hiding this comment.
It seems like de-duplication needs to be figured out first. I would hate for someone to be unable to pull a transitive dependency because they used rules_helm and now necessary repositories are missing. Would it be reasonable to prefix the dependencies with the name of the module and then require users define the name of a hub repository where these deps are accessed that needs to be globally unique? I also am still fairly new to bzlmod but isn't it supposed to solve for issues like this in transitive dependencies?
|
@abrisco Can you please take another look? Thanks! |
abrisco
left a comment
abrisco
left a comment
There was a problem hiding this comment.
Sorry this fell off my radar! More questions for you 😄
| # And the _register_toolchains needs to be called *after* iteration of all modules. | ||
| for module in ctx.modules: | ||
| for repository in module.tags.import_repository: | ||
| if not module.is_root: |
There was a problem hiding this comment.
It seems like de-duplication needs to be figured out first. I would hate for someone to be unable to pull a transitive dependency because they used rules_helm and now necessary repositories are missing. Would it be reasonable to prefix the dependencies with the name of the module and then require users define the name of a hub repository where these deps are accessed that needs to be globally unique? I also am still fairly new to bzlmod but isn't it supposed to solve for issues like this in transitive dependencies?
3 participants
No description provided.