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 tool for config file validation and envvar replacement #94

Merged
merged 31 commits into from
Aug 30, 2024

Conversation

tsloughter
Copy link
Member

No description provided.

@tsloughter
Copy link
Member Author

Opening this even though I don't like the solution to the schema needing to not be a parent of the go file solution it uses of requiring use of a Makefile. But since there wasn't a meeting this week and I haven't gotten any responses on slack I figured I'd just open the existing solution and go from there with restructuring.

So I haven't bothered adding the github actions tests and publishing releases stuff.

@tsloughter tsloughter marked this pull request as ready for review June 18, 2024 10:21
@tsloughter tsloughter requested a review from a team June 18, 2024 10:21
@tsloughter
Copy link
Member Author

I marked this as ready for review because maybe that is why it wasn't getting any reviews...

I still don't like it though. I think the structure is wrong but wanted feedback, esp from Go users, on how to better structure it. But also in general if it would be ok that the code was just interspersed with the rest of the repo (as in, put main.go at the top level so it can read schema/* without it being copied to a subdirectory).

Copy link
Member

@marcalff marcalff 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 providing some tooling, looking to use it.

According to the spec, environment variable substitution should not be recursive.

validator/main.go Outdated Show resolved Hide resolved
validator/LICENSE Outdated Show resolved Hide resolved
validator/Makefile Show resolved Hide resolved
validator/README.md Outdated Show resolved Hide resolved
validator/go.mod Show resolved Hide resolved
validator/main_test.go Show resolved Hide resolved
validator/shelltests/help.test Outdated Show resolved Hide resolved
@tsloughter
Copy link
Member Author

Update: I wasn't properly supporting types as the library was turning stuff like disabled: ${OTEL_SDK_DISABLED:-false} to disabled: "false", but I think I've got it all resolved. Just gotta clean up my mess of Go code and will push up the fix tomorrow morning probably.

@tsloughter
Copy link
Member Author

I think issues with substitution have been resolved. Only thing is it now prefixes substituted values with the tag in yaml:

attribute_value_length_limit: !!int 0xdeadbeef

@tsloughter
Copy link
Member Author

When this is accepted as the path forward I can add github action based publishing of cli binaries and docker image.

.github/workflows/validator-tests.yaml Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
validator/main_test.go Show resolved Hide resolved
validator/Makefile Show resolved Hide resolved
validator/README.md Outdated Show resolved Hide resolved
@jack-berg
Copy link
Member

When this is accepted as the path forward I can add github action based publishing of cli binaries and docker image.

I think this is a reasonable direction. We can always iterate on the technologies used under the covers since they are abstracted away behind the docker interface. This provides a useful capability for end users and SDK implementations alike.

@tsloughter
Copy link
Member Author

@jack-berg ok, I've added binary and docker publish support to the github actions.

Docker images are published to ghcr. Binary releases are built on tags with a tool called cargo-dist (originally built for building binaries with cargo but now supports generic commands to compile the binary, I've been using this on a couple other projects recently).

Open questions:

  • Do we want a homebrew recipe? cargo-dist will generate it and publish to a tap if we get open-telemetry/homebrew-tap repo created. I figured unnecessary to start, but it can easily be added if deemed useful and they agree to create the repo.
  • Should I get rid of the install script and the user just have to download the right binary? My main issue with the install script is you need a path to install the binary and there is no XDG_ support builtin. What I used is "[$OTEL_CONFIG_VALIDATOR_HOME/bin", "~/.otel_config_validator/bin"], which means if user defines $OTEL_CONFIG_VALIDATOR_HOME it will install there otherwise "~/.otel_config_validator/bin" -- but I hate polluting ~/ with dotfiles.
  • I haven't tested Powershell yet. I may just remove for now and we can add it back later if needed?
  • Docker images are published to ghcr. Does it need to go to docker hub or like a CNCF quay or something?
  • Docker images are published with docker tags of the full version major.minor.patch, plus major.minor and major. Should we only publish for the full version?
  • Images are published on merge to main with just the sha git ref as the docker tag. This can make it simpler for people to test between releases but does greatly increase the number of published images and may be unnecessary. Should I remove it?

@jack-berg
Copy link
Member

@tsloughter can we go through these in person at the next config SIG meeting on 8/5? Not sure if you can make it, but seems like it'd be easier than working through these questions async.

@tsloughter
Copy link
Member Author

@jack-berg yes, and agreed. I can do my best to make the meeting on Monday. Likely can only do half the meeting, but won't know which half until closer to start time...

Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Would it be useful to use the same tools as the collector uses for shipping binaries of the collector? goreleaser has worked pretty well and does simplify the github action configuration needed

You can see an example of the configuration https://github.com/open-telemetry/opentelemetry-collector-releases/blob/main/.github/workflows/base-ci-goreleaser.yaml

I would also be happy to help move to that tool if its deemed useful

@tsloughter
Copy link
Member Author

@jack-berg I finally updated to accept a schema as an option to the cli. Also broke the version from versioning with the schema and added a prefix validator- required for tags that will trigger publishing of the releases and its assets.

Something I'd like to add, but can be added after a merge, is a command to print the schema version that is embedded in the tool. But it doesn't seem the version is anywhere in the schema itself? A version of the schema is based solely on the git tag it is associated with? That makes it tough.. I was hoping it was just a field I could read from schema/opentelemetry_configuration.json. I suppose I could also hardcode it into main.go and require it be updated each time a new schema is released.

@tsloughter
Copy link
Member Author

@codeboten I'm fine with whatever. I used cargo-dist because I had previous experience with it from another project and really liked it, then used it again on a Go project so knew it worked for those.

I take it there is a Pro plan that could be used, since most features are behind the Pro plan (ones being used here with cargo-dist)?

At least looks like it? https://github.com/open-telemetry/opentelemetry-collector-releases/blob/main/.github/workflows/base-ci-goreleaser.yaml#L69

@codeboten
Copy link
Contributor

I take it there is a Pro plan that could be used

Right, we use the pro version for the collector releases. I don't have a strong preference other than that i'm more familiar with goreleaser 😅

Copy link
Member

@jack-berg jack-berg left a comment

Choose a reason for hiding this comment

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

I have a few small lingering questions but this seems like a great start.

validator/Makefile Show resolved Hide resolved
validator/README.md Outdated Show resolved Hide resolved
validator/README.md Outdated Show resolved Hide resolved
validator/Dockerfile Outdated Show resolved Hide resolved
validator/README.md Outdated Show resolved Hide resolved
validator/README.md Show resolved Hide resolved
.github/workflows/validator-release.yml Show resolved Hide resolved
.github/workflows/validator-docker.yaml Show resolved Hide resolved
@tsloughter
Copy link
Member Author

@jack-berg I updated the Dockerfile to build in the binary and the shelltests to be able to use a bin from anywhere (not just ./otel_config_validator. As well as the github action to use a new makefile target that builds/runs the docker image to execute the shelltests. Plus, updated the readme to show using the makefile to run the shelltests (so it uses docker as well, no need to install anything else).

To support this change the runner now copies the built
cli from the builder stage of the main Dockerfile and
the Dockerfile.shelltest was removed.

To support running in both local environments and
Docker the shelltests depend on the cli being in the
path and the Docker image adds `/` to the PATH, where
it copes the binary from the first stage, and the
makefile run of the otel_config_validator on the
examples will add the validator dir to the path at
the end so it is only used if `/otel_config_validator`
isn't in the PATH.
@jack-berg jack-berg merged commit 82547e9 into open-telemetry:main Aug 30, 2024
9 checks passed
@jack-berg jack-berg mentioned this pull request Aug 30, 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.

CLI to expand variables, validate and export to json (or yaml)
4 participants