Codecov Report

Base: 91.20% // Head: 91.18% // Decreases project coverage by -0.01% ⚠️

Coverage data is based on head (b93a46f) compared to base (64fc4de).
Patch coverage: 100.00% of modified lines in pull request are covered.

@@            Coverage Diff             @@
##             main    #6470      +/-   ##
==========================================
- Coverage   91.20%   91.18%   -0.02%     
==========================================
  Files         245      246       +1     
  Lines       14194    14196       +2     
==========================================
  Hits        12945    12945              
- Misses        999     1001       +2     
  Partials      250      250              

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

This PR was marked stale due to lack of activity. It will be closed in 14 days.

As I said on the PR description, I want to make sure we don't give up on this option just because of breaking changes: we have done many Go API breaking changes recently and I think it can be justified to do it here too if this is a better solution than struct tags. We have to think about the long term: the majority of users of this API are in the future, not the present so if we can improve things now, even at a small annoyance for current users, I think this should be the way to go.

The main two benefits I see of this option are:

1. We have a smaller maintenance burden with this option: we only need to maintain the marshaling method on the type instead of having to provide our custom marshaling that detects struct tags for our users
2. We can express opaqueness with higher granularity: this PR has the Headers type but we (and end-users) could have just used map[string]configopaque.String. A struct tags solution would need a special case for this kind of issue.
3. We don't have to deal with error modes that would come in struct tags, e.g. if you add the wrong struct tag or use the incorrect type you get an error at runtime, with types the error happens at compile time.

I think this PR is representative of the changes that would be needed in contrib for updating each component (scaled by the number of components there that is :))

I left only the configopaque package here. The process I propose for changing a field type in several steps is:

1. (Release v0.N) Add a new OpaqueField field with the new type and deprecate Field in favor of it. Modify code to take into account both fields (and log a warning). Make it so OpaqueField has the struct tag that Field used to have.
2. (Release v0.N+1) Change Field to have opaque type and remove deprecation, deprecate OpaqueField in favor of Field. Move the struct tag back to Field.
3. (Release v0.N+2) Remove (deprecated) OpaqueField field.

This would apply to both the header fields from configgrpc/confighttp structs as well as fields from contrib config structs. I think we want to make sure only one of the fields is settable when passing YAML config

If this makes sense, I will open a separate PR for doing this for headers on the confighttp/configgrpc structs and generate a list of fields on contrib after that.

Sounds like a plan!

If it helps on reviewing this PR, 563a9c5 from #6637 shows the first step for confighttp