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 template-level parameters and file_name per template config #294

Merged
merged 12 commits into from
Aug 12, 2024

Conversation

lquerel
Copy link
Contributor

@lquerel lquerel commented Aug 6, 2024

This PR adds two new optional fields in the templates section of a Weaver configuration file:

  • params: Defines a map of key-value pairs which will be merged with the ones defined at the top level of the weaver.yaml file. CLI parameters can also be used to override these parameters.
  • file_name: Defines the relative file path of the output generated from the template. This field is a Jinja expression which will be evaluated with the same context used in the template itself. The parameters mentioned above are also accessible.

An example of usage:

params:
  exclude_deprecated: true
  
templates:
  - pattern: attribute.py
    params:
      exclude_stability: ["stable"]
    filter: "semconv_grouped_attributes($params)"
    application_mode: each
    file_name: "incubating/{{ctx.root_namespace | snake_case}}.py"
  - pattern: attribute.py
    params:
      exclude_stability: ["experimental"]
    filter: "semconv_grouped_attributes($params)"
    application_mode: each
    file_name: "{{ctx.root_namespace | snake_case}}.py"

Note: params are now merged in an additive way. For example, if a parameter is defined in the params section of a template, then the final params section for this template will include both the params inherited from the file-level configuration and the params defined in the template configuration. If the override is a parameter set to null, then this parameter is removed from the final params section. This modification should not have a significant impact on the existing codegen, as I don't believe any codegen was relying on this specific behavior.

Closes open-telemetry/weaver#288.

@lquerel lquerel added the enhancement New feature or request label Aug 6, 2024
@lquerel lquerel self-assigned this Aug 6, 2024
@lquerel lquerel marked this pull request as ready for review August 7, 2024 21:47
@lquerel lquerel requested a review from jsuereth as a code owner August 7, 2024 21:47
@lquerel lquerel changed the title [WIP] Add template-level parameters and file_name per template config Add template-level parameters and file_name per template config Aug 7, 2024
Copy link
Contributor

@jsuereth jsuereth left a comment

Choose a reason for hiding this comment

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

Overall looks good BUT I find the weaver config confusing.

I think we should deprecated pattern for template.

Specifically:

templates:
  - pattern: test.md
    filter: .
    application_mode: single
    params:
      template_name: test1
      test_1: true
    file_name: "{{params.template_name | snake_case}}.json"

Looks confusing, since test.md is the template you're using, not a pattern at all.

Instead, let's do:

templates:
  - template: test.md
    filter: .
    application_mode: single
    params:
      template_name: test1
      test_1: true
    file_name: "{{params.template_name | snake_case}}.json"

I think we can do this in a non-breaking way by deprecating the "pattern" attribute and adding "template" where we pull from pattern if it exists, otherwise use template.

crates/weaver_semconv_gen/Cargo.toml Outdated Show resolved Hide resolved
crates/weaver_forge/src/lib.rs Show resolved Hide resolved
@lquerel
Copy link
Contributor Author

lquerel commented Aug 9, 2024

I think we can do this in a non-breaking way by deprecating the "pattern" attribute and adding "template" where we pull from pattern if it exists, otherwise use template.

Can be done with a serde alias. I will also update the docs.

@lquerel
Copy link
Contributor Author

lquerel commented Aug 9, 2024

@jsuereth I renamed templates.pattern to templates.template and added pattern as a serde alias of template for backward-compatibility. I also updated documentations and examples.

@lquerel lquerel requested a review from a team August 11, 2024 16:55
@lquerel lquerel merged commit e66e1b6 into open-telemetry:main Aug 12, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Add support for a params section per template
2 participants