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 documentation-related sources to helm_chart #15638

Merged
merged 3 commits into from
Apr 13, 2024

Conversation

alonsodomin
Copy link
Contributor

Closes #15622

kaos
kaos previously approved these changes May 25, 2022
Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

So unfortunately it's an antipattern and not likely to work to have multiple source fields on a target :( See https://pantsbuild.slack.com/archives/C01CQHVDMMW/p1653441966623019

Can these all be in the same sources field?

@kaos kaos dismissed their stale review May 25, 2022 19:37

Good catch by Eric, I overlooked that all fields were based on source field.

@alonsodomin
Copy link
Contributor Author

I see what you mean, I guess I could include several of those in the sources field... however I was considering this a stepping stone into being able to generate the README.md by adding support for a tool like helm-docs.

So I was creating the readme field because I will need a specific field to be used in the output field for GenerateSourcesRequest and then I presume I got carried away and included a field for each of the other ones.

So, considering the possibility of adding support to generate the README.md file, what could be a better approach?

@kaos
Copy link
Member

kaos commented May 26, 2022

I'm not sure if this is best-practice, but it works for the Docker backend.

class GenerateDockerfileRequest(GenerateSourcesRequest):
# This will always run codegen when hydrating `docker_image`s, performing source validations but
# does not generate anything if there are no `instructions` defined on the target.
input = DockerImageSourceField
output = DockerImageSourceField

In this case, using the above, you could have a readme StringField, and when set, run helm-docs to create the file (or just a bool field if the output name should be fixed).

helm_chart(
  name="my-chart",
  generate_docs=True,
  ...
)

@alonsodomin
Copy link
Contributor Author

alonsodomin commented May 27, 2022

Actually the field name needs to be fixed, and similar thing happens with the other fields...

Another thing I'm considering is if it's worth adding a helm_docs target because the helm-docs tool supports an optional source file to be used as the template.

Maybe in that case I can plug the GenerateSourcesRequest output field to a FileSourceField, which should already be supported by current helm backend.

@cognifloyd cognifloyd changed the title Add documentation-related source fields to helm_chart Add documentation-related sources to helm_chart Apr 13, 2024
@cognifloyd cognifloyd dismissed Eric-Arellano’s stale review April 13, 2024 02:18

Review was addressed: This now only adds default sources, not additional source fields.

@Eric-Arellano
Copy link
Contributor

This now only adds default sources, not additional source fields.

Looks good! If you haven't yet, double check the PR title still is appropriate. Seems fine to me, but not sure if you'd reword it since the approach changed.

@cognifloyd cognifloyd merged commit 8f44568 into pantsbuild:main Apr 13, 2024
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for Helm chart documentation files
4 participants