fix: don't assume bash when displaying interactive CLI prompts #21839
Conversation
danxmoran
force-pushed
the
dm-upgrade-interactive-prompt-powershell-21838
branch
from
ddb9b4f
to
ac22894
Compare
|
|
| AuthToken: options.target.token, | ||
| Org: options.target.orgName, | ||
| Bucket: options.target.bucket, | ||
| Retention: options.target.retention, |
There was a problem hiding this comment.
It's a bit confusing that the prompt appears to be for a different kind of string than the options value now? (nanos vs hours?)
There was a problem hiding this comment.
The help-text isn't very helpful, but I think the behavior is the same before and after this change. Whatever gets passed to --retention gets parsed as a time.Duration
- In the old impl: https://github.com/influxdata/influxdb/pull/21839/files#diff-ec11f92a3df5beb5cc1657892bfc169a49bed091ce94ebd1fae3d989a8c6eceeL43
- In the new impl: https://github.com/influxdata/influx-cli/blob/main/clients/setup/setup.go#L141
I can update the description in the help-text to show examples of durations (i.e. 72h)
| } | ||
| if req.Bucket == "" { | ||
| req.Bucket = internal.GetInput(ui, "Please type your primary bucket name", "") | ||
| if cliReq.Password != nil { |
There was a problem hiding this comment.
So for influxdb.OnboardingRequest we don't distinguish between nil and default-value, but for api.OnboardingRequest we do?
There was a problem hiding this comment.
Yes, a consequence of codegen vs. hand-rolled. The codegen'd models use pointer-fields for every optional field.
There was a problem hiding this comment.
Talked on Slack: going to try deleting influxdb.OnboardingRequest and migrating existing code to use the codegen'd model imported from the CLI
There was a problem hiding this comment.
After trying, would prefer to leave it as an adapter layer here. Both influxdb.OnboardingRequest and api.OnboardingRequest embed references to other models from their respective packages; if you change the top-level type, you also have to change how you process all the fields, and it starts to sprawl into many files.
danxmoran
force-pushed
the
dm-upgrade-interactive-prompt-powershell-21838
branch
from
4949f0b
to
1a982a9
Compare
|
Reset and force-pushed to clean up the git history, not sure what happened with all the merges from master |
Closes #21838
Requires influxdata/influx-cli#192 to merge first so we can re-updatego.modto point atmain