Create diag package and switch all applicable []tfprotov6.Diagnostic usage #110
Conversation
bflad
force-pushed
the
bflad-f-diagnostics
branch
2 times, most recently
from
6c2a877 to
fcb754d
Compare
| // | ||
| // Usage of this method outside the framework is not supported nor considered | ||
| // for backwards compatibility promises. | ||
| func (diags Diagnostics) ToTfprotov6Diagnostics() []*tfprotov6.Diagnostic { |
There was a problem hiding this comment.
two things:
- This doesn't appear to require any privileged access to the
diagpackage or benefit from being a method on theDiagnosticstype. Is there a reason we shouldn't just make it a function intfsdk? - I think
Tfprotov6in this may be unnecessarily verbose.ToProto6Diagnosticsis probably sufficient; we know which protocol we're talking about, and we're unlikely to be talking about any other protocol's diagnostics except Terraform's.
There was a problem hiding this comment.
This doesn't appear to require any privileged access to the diag package or benefit from being a method on the Diagnostics type. Is there a reason we shouldn't just make it a function in tfsdk?
It does keep the diagnostics implementation altogether.
There was a problem hiding this comment.
Hm. Is that worth having an exported method we don't want people to call? 🤔 (I don't know the answer, I'm wondering out loud.)
There was a problem hiding this comment.
Another option here would be to move that exported function into a separate internal package, maybe along the lines of diag/internal/convert. I'd be more apt to do this so at least the code is tangential the other diagnostics code. I'd be worried about import cycles if we tried to setup something more generic like internal/toproto6.
@kmoe do you have any opinions?
There was a problem hiding this comment.
I'd still prefer this to be unexported as we don't expect provider developers to use it, and I'm worried about what they'll do with this unintended compatibility interface, but I'm not gonna hold this up over it.
bflad
force-pushed
the
bflad-f-diagnostics
branch
from
faa9b33 to
9da1f1b
Compare
|
Rebased after #102 |
bflad
force-pushed
the
bflad-f-diagnostics
branch
from
22441b3 to
cdee112
Compare
…except where interfacing directly with the underlying protocol type
Co-authored-by: Paddy <paddy@hashicorp.com>
bflad
force-pushed
the
bflad-f-diagnostics
branch
from
cdee112 to
9517ae4
Compare
Co-authored-by: Paddy <paddy@hashicorp.com>
| return resp, nil | ||
| } | ||
|
|
||
| // validateProviderConfigResponse is a thin abstraction to allow native Diagnostics usage |
There was a problem hiding this comment.
nitpick: ideally we'd call out why this allows native Diagnostics usage, but we can fast-follow with that.
* Don't recursively validate SingleNestedAttribute if it is Null. * Add CHANGELOG entry. * Use 'o.Null' check. * Also check for 'o.Unknown'. * Update .changelog/118.txt Co-authored-by: Paddy <paddy@carvers.co> * Use diag package helpers (#110). Co-authored-by: Paddy <paddy@carvers.co>
|
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
Pending #108
Closes #24
This is an implementation of diagnostics as an interface type. It is intended to serve as a discussion starting point and may be thrown away or updated depending on the outcomes of #108.