Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add extension to allow bazelmod style handling of dependencies #124

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

Ongy
Copy link

@Ongy Ongy commented Dec 20, 2024

No description provided.

Copy link
Contributor

@mering mering left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking the time to proposing this upstream!

helm/extensions.bzl Outdated Show resolved Hide resolved
helm/extensions.bzl Outdated Show resolved Hide resolved
helm/extensions.bzl Outdated Show resolved Hide resolved
helm/extensions.bzl Outdated Show resolved Hide resolved
Copy link
Owner

@abrisco abrisco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay here! @Ongy what do you think about the review from @mering? I think this change is fine as is but the feedback also sounds good to me!

@Ongy Ongy requested a review from abrisco January 7, 2025 09:01
helm/extensions.bzl Outdated Show resolved Hide resolved
helm/extensions.bzl Outdated Show resolved Hide resolved
tag_classes = {"options": options},
tag_classes = {
"options": options,
"import_repository": tag_class(attrs = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@abrisco WDYT?

helm/extensions.bzl Outdated Show resolved Hide resolved
@Ongy Ongy force-pushed the add-import-repository-extension branch from 0572050 to eebaf6d Compare January 8, 2025 12:18
helm/extensions.bzl Outdated Show resolved Hide resolved
@Ongy Ongy force-pushed the add-import-repository-extension branch from 0124e8d to 988121b Compare January 24, 2025 08:53
@Ongy
Copy link
Author

Ongy commented Jan 24, 2025

@abrisco can you take a look at the naming discussion, and do another pass over the code? thx.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants