Add warning message for deprecated "hashicorp/terraform" provider #111
Conversation
…orm to have features parity with terraform. Signed-off-by: Dmitry Kisler <[email protected]>
|
Hey @kislerdm , thanks for getting this done so quickly! How would you feel about moving this logic out to it's own package? I think a small package with a function that accepts a namespace + type, and returns errors based on that would be amazing. In the long run this package could be swapped out easily for something else. Part of the reason that I raise this is that I feel like it doesn't really belong as a function dangling on |
|
@Yantrio Hey James! Thanks for a prompt review.
It's a very reasonable proposal indeed. I'll do the refactoring according to it. Also, post-alpha, we shall figure how to govern warnings for any arbitrary provider. In other words, how can we make sure that we, as a governing body between the provider's user and the provider's maintainers, keep warnings and any additional provider's info consistently with intentions of the provider's maintainer. For example, shall we warn tofu users when the provider they rely on is fetched from the archived github repo, a/k/a "deprecated"?
You are absolutely right that WDYT about the module refactoring after the alpha release? I'd like to propose its simplification and clear boundary separation between every app layer. Frankly, our app feel over-complex despite the size of the problem it solves. I highlighted the proposal in the PR description as
The motivation:
|
Signed-off-by: Dmitry Kisler <[email protected]>
Signed-off-by: Dmitry Kisler <[email protected]>
|
I like this approach, especially passing the warnings through the context to simplify the function sigs everywhere. I'm happy to merge this. When it comes to the post-alpha comments above and the TODOs in your code, I think we should raise some issues on this repo so we don't forget them for the long term solution, and each of those can be considered individually at a later date. As for refactoring, I think it's a good idea and we should look into this, can you raise an issue with some of your comments around this please? |
|
@kislerdm Would you mind fixing the linting issues before we merge please? 🙏 Thanks |
Signed-off-by: Dmitry Kisler <[email protected]>
Definitely! I also created the issue "placeholder" to configure linting explicitly. |
Signed-off-by: Dmitry Kisler <[email protected]>
details: https://github.com/opentofu/registry/pull/111/files#r1341757605 Signed-off-by: Dmitry Kisler <[email protected]>
src/internal/warnings/warnings.go
Outdated
| return nil | ||
| } | ||
|
|
||
| var warningsContext = struct{}{} //nolint:gochecknoglobals // This is a commonly used pattern for context binding |
There was a problem hiding this comment.
Hey! Thanks for suggestion, it does make sense indeed! I've implemented the change accordingly, however the key is called contextKey because it's obvious that it relates to warnings as it's internal to the warnings package. Please review the change and resolve the comment. Thanks!
There was a problem hiding this comment.
TBH I'm not a huge fan of this Context-based approach, a list of warnings doesn't strike me as something you'd want to implicitly pass through the context.
Couldn't we just add (namespace, provider) as parameters to the versionsResponse function, and then call ProviderWarnings from inside that function? That sounds a fair bit simpler.
Interestingly, it seems like the Terraform Provider Error Diags package aims to solve this exact problem: https://pkg.go.dev/github.com/hashicorp/terraform-plugin-sdk/v2/diag#Diagnostics
Anyway, I don't particularly care either way, since this is alpha code anyway. I might be missing something problematic about passing the namespace/provider directly to the versionsResponse function?
|
@cube2222 hey Kuba, thanks for your review! I support your point of view on explicit declaration, however context binding is a commonly used pattern to propagate information without modifying signatures. In this particular case my motivation was to modify signatures of the least number of functions even though they are private. All in all, your proposal is valid - it'd only require to modify the signature of the "fetchFromGithub" function, and propagate "namespace" and "type" to the "response" function. So such modification would allow us to remove some logic and avoid its maintenance - definitely worthwhile. However, maybe we can come back to this decision post alpha release? If I understood your comment here, we will have to make more critical decisions about registry. WDYT? |
|
BTW the tf diagnostics is also singleton, but with a better defined concern. We could split warnings propagation into a dedicated diag context moving forward. For example, it'd enable us to report if the provider's repo was archived - find details here. |
…created. details: opentofu/registry#111 (comment) Signed-off-by: Dmitry Kisler <[email protected]>
Now, the caching strategy implementation is more clear. When the cache-hit: return, otherwise: check existence of the repo, fetch the versions from the source and populate the cache. Signed-off-by: Dmitry Kisler <[email protected]>
|
Hey folks! I refactored the implementation for the sake of simplicity:
|
|
Hey @Yantrio! Not at all! I'll fix linting error tomorrow. |
Signed-off-by: Dmitry Kisler <[email protected]>
|
@Yantrio Hey James! The fix addressing the nil-pointer issue was implemented. Please have a look. Thanks! |
Closes #108
What changed
Why do we need it
Followups
Consider the ports-adapters pattern to facilitate testability, i.e. define adapters to the clients using interfaces.
Add more unit tests. Even if the logic seems simple, there is still a handful of conditions, we can do better when it comes to testing.