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

Ability to specify additional files to be added to the chart #102

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

Conversation

dormidon
Copy link
Contributor

@dormidon dormidon commented Oct 1, 2024

Try to address #74

  • files attribute added to helm_package
  • it receives map from fileset to directory inside a chart where the whole fileset will be copied

Could be used as


filegroup(
    name = "configs",
    srcs = glob([
        "configs/**",
    ]),
)

helm_package(
    name = "package",
    files = {
        ":configs": "configs",
    },
    # rest of target parameters
)

In this case all file structure under configs directory will be copied to configs directory "inside" chart and would be available via .Files.Get ("configs/my-config.yaml")

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.

Thanks for the change and sorry for the delay on the review! Out of curiosity, why not add the files field that builds a runfiles-like directory? It seems like that would make loading files more consistent but would also mean your charts will probably be different in and out of bazel if you're trying to continue to support both.

@dormidon
Copy link
Contributor Author

Hello!

Thank you for the feedback.

Out of curiosity, why not add the files field that builds a runfiles-like directory?

It seems that the answer is simple - because of lack of experience with Bazel :)
Could you please provide more details about your idea how it could be improved?

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.

Hello!

Thank you for the feedback.

Out of curiosity, why not add the files field that builds a runfiles-like directory?

It seems that the answer is simple - because of lack of experience with Bazel :) Could you please provide more details about your idea how it could be improved?

I've left a comment with hopefully a useful suggestion!

@@ -162,11 +162,15 @@ def _helm_package_impl(ctx):

args.add("-workspace_name", ctx.workspace_name)

for ln in ctx.attr.files:
Copy link
Owner

Choose a reason for hiding this comment

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

Kinda hard to use suggestions to suggest exactly what I'm thinking but:

Suggested change
for ln in ctx.attr.files:
def _rlocationpath(file, workspace_name):
if file.short_path.startswith("../"):
return file.short_path[len("../"):]
return "{}/{}".format(workspace_name, file.short_path)
workspace_name = ctx.workspace_name
def _rlocationpath_mapping(file):
return "-add_files={}={}".format(file.path, _rlocationpath(file, workspace_name))
args.add_all(ctx.files.files, map_each = _rlocationpath_mapping, allow_closure = True)

Anyone familiar with Runfiles APIs (e.g. rust, python) should have an easy time finding files and not need to refer to the BUILD file target to understand what files are included and where to find them.

purkhusid added a commit to purkhusid/rules_helm that referenced this pull request Dec 16, 2024
## What
This is a continuation of abrisco#102 with a bit nicer API

This allows the user to add arbitrary source of generated files to the helm chart at their desired location.
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.

2 participants