[any-tf-provider] Switch args parsing to cobra #2742
Conversation
iwahbe
force-pushed
the
iwahbe/dynamic-switch-to-cobra
branch
from
316be51 to
c61588b
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2742 +/- ##
==========================================
- Coverage 69.64% 69.62% -0.02%
==========================================
Files 301 302 +1
Lines 38726 38736 +10
==========================================
Hits 26969 26969
- Misses 10240 10250 +10
Partials 1517 1517 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Approving because of time zone differences.
I really like keeping test utils in their own module, for the record, because it makes them much more discoverable. But I also ideally want that to be part of a concerted effort to consolidate and centralize such utilities. I worry that adding them in this way would not facilitate that.
| cmd.Flags().StringVar(&upstreamRepoPath, "upstreamRepoPath", "", | ||
| "Specify a local file path to the root of the Git repository of the provider being dynamically bridged") | ||
|
|
||
| // We hide docs flags since they are not intended for end users, and they may not be stable. |
There was a problem hiding this comment.
How are we going to make these flags discoverable for maintainers? I've gotten burned by this before and would like an alternative.
There was a problem hiding this comment.
I'm not sure, but I'm open to suggestions.
There was a problem hiding this comment.
Two options, really:
- do not hide the flags and clarify in the help text to not use them
- put the documentation in a discoverable place for maintainers (possibly the README for this module, though discoverability is always difficult to predict)
| if out.Len() == 0 { | ||
| return | ||
| } | ||
| tfbridge.GetLogger(ctx).Info(out.String()) |
There was a problem hiding this comment.
Why do we need this?
There was a problem hiding this comment.
Because the CLI hides raw stdout unless the provider panics. This ensures that output (like help text) is printed.
There was a problem hiding this comment.
Sounds like it would be worth adding a comment to that effect.
There was a problem hiding this comment.
Oh, interesting. This is the pulumi cli?
This allows modules outside of the bridge to test code that uses `tfbridge.GetLogger`. This will be used by `dynamic`. It may also be used by AWS, which currently has test code which uses the now moved-to-internal logging.InitContext.
This gets us out of the business of writing flag parsing. This does change the format of options. Before, they looked like: ```console $ pulumi package get-schema registry.opentofu.org/owner/provider version fullDocs=true ``` They now look like: ```console $ pulumi package get-schema -- registry.opentofu.org/owner/provider version --fullDocs ```
iwahbe
force-pushed
the
iwahbe/dynamic-switch-to-cobra
branch
from
c61588b to
04fb870
Compare
| @@ -0,0 +1,7 @@ | |||
| # bridgetest | |||
There was a problem hiding this comment.
do you have plans to use this outside of the dynamic module? I do agree with @guineveresaenger that it's a little surprising to make a new top level package to hold this in the context of this PR.
Simplify args parsing by using cobra.
My hope is that this will make the implementation of #2717 much easier.