honeycombio / terraform-honeycombio-opentelemetry-starter-pack

A terraform starter pack for creating honeycomb nouns for standard opentelemetry data
Other
5 stars 2 forks source link

Remove golden and otel from naming and use OpenTelmetry everywhere #3

Closed tdarwin closed 1 year ago

tdarwin commented 1 year ago

Short description of the changes

This PR changes the name from golden-otel-starter-pack to opentelemetry-starter-pack which will be a better name to publish under.

How to verify that this has the expected result

Make sure golden is no longer in the repo, and that any case of OTEL is actually spelled out as OpenTelemetry

jharley commented 1 year ago

I missed the initial PR, so just catching some things now:

This line should be something like:

  version = ">= 0.11.2"

Otherwise you will be updating the module with every new release of the provider.

I'm also a bit concerned about passing around the API key everywhere when we're really only using the Honeycomb provider? This is not a pattern generally in use: we don't see AWS modules passing around creds but just relying on the provider's environment to Do The Right Thing™

Modules also generally just take the required_providers block and don't redeclare a provider block themselves: this is done by the configuration making use of the module.

irvingpop commented 1 year ago

@jharley I agree with your comment about making the provider version flexible, that's a good call and will help reduce the maintenance burden. but also let's not creep the scope of this PR 🙂

I'm not sure what you mean about the API keys though, perhaps we can discuss that separately.

tdarwin commented 1 year ago

@jharley you're talking about this bit in the main.tf?

provider "honeycombio" {
  api_key = var.honeycomb_api_key
  # You can supply this via the environment variable HONEYCOMB_API_KEY or by setting the value in a .tfvars file
}

I just had that there as it was part of the refinery starter pack, but I see it's not part of the build-events starter pack so I don't mind pulling it out. But I think I'll make a separate PR for that bit as this was supposed to be about renaming.

jharley commented 1 year ago

Yep!

I just had that there as it was part of the refinery starter pack

Right! The difference between this and the refinery (and some of the AWS integrations) is that the key is needed to be injected into configurations later. As you just need a key to make API calls, I'd avoid using it this way.