Closed tsloughter closed 2 months ago
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).
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.
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
When this is accepted as the path forward I can add github action based publishing of cli binaries and docker image.
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.
@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:
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.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.major.minor.patch
, plus major.minor
and major
. Should we only publish for the full version?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?@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.
@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...
@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.
@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
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 😅
@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).
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.