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

[config] introduce config package #4376

Merged
merged 7 commits into from
Oct 10, 2023

Conversation

codeboten
Copy link
Contributor

@codeboten codeboten commented Oct 3, 2023

This change introduces the config package with the following changes:

  • add go-jsonschema tool to tools.go
  • add make genjsonschema target to Makefile
  • run the genjsonschema target to generate the code from the opentelemetry configuration schema
  • add a readme for the package
  • run dependabot-generate

This is a scoped down version of #4228

Note that the generated model code lives in a package that includes the version of the schema in the package name. I'm not sure if that's the right thing to do, but it is similar to what is done for semconv.

Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

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

Note that the generated model code lives in a package that includes the version of the schema in the package name.
I'm not sure if that's the right thing to do, but it is similar to what is done for semconv.

With semconv we intented to support all kind of versions. For the configuration, I suppose it is OK to support the latest configuration schema version. Therefore, I do not think we should include a version in the package name. Similar PR: open-telemetry/opentelemetry-go#4538

config/README.md Show resolved Hide resolved
Makefile Show resolved Hide resolved
tools/go.mod Show resolved Hide resolved
config/README.md Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@pellared pellared dismissed their stale review October 10, 2023 15:40

Dismissing my previous review after a chat with MrAlias.

Alex Boten added 6 commits October 10, 2023 09:26
This change introduces the config package with the following changes:

- add go-jsonschema tool to tools.go
- add `make genjsonschema` target to Makefile
- run the genjsonschema target to generate the code from the opentelemetry configuration schema
- add a readme for the package

Signed-off-by: Alex Boten <aboten@lightstep.com>
Signed-off-by: Alex Boten <aboten@lightstep.com>
Signed-off-by: Alex Boten <aboten@lightstep.com>
Signed-off-by: Alex Boten <aboten@lightstep.com>
Signed-off-by: Alex Boten <aboten@lightstep.com>
Signed-off-by: Alex Boten <aboten@lightstep.com>
@codeboten codeboten force-pushed the codeboten/genjsonschema branch from 27a4eec to d8cf141 Compare October 10, 2023 16:30
@codeboten
Copy link
Contributor Author

@pellared i've moved generated_config.go as per the suggestion and reverted the change on tools/go.mod to remove the toolchain directive

Signed-off-by: Alex Boten <aboten@lightstep.com>
@codeboten
Copy link
Contributor Author

@pellared well i tried removing the toolchain directive and it caused a lint failure: https://github.com/open-telemetry/opentelemetry-go-contrib/actions/runs/6472155249/job/17572209023?pr=4376

I've re-added it, though since it's automatically added I didn't update it to 1.21.2. Appreciate any thoughts/advice here

@MadVikingGod MadVikingGod merged commit 4ec3201 into open-telemetry:main Oct 10, 2023
21 checks passed
@codeboten codeboten deleted the codeboten/genjsonschema branch October 10, 2023 18:46
codeboten pushed a commit to open-telemetry/opentelemetry-collector that referenced this pull request Nov 16, 2023
This code will live in the opentelemetry-go-contrib repo in the future.

This depends on
open-telemetry/opentelemetry-go-contrib#4376

---------

Signed-off-by: Alex Boten <aboten@lightstep.com>
@pellared pellared added this to the untracked milestone Nov 8, 2024
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.

4 participants