Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: don't assume bash when displaying interactive CLI prompts #21839

Merged
merged 1 commit into from
Jul 14, 2021
Merged

fix: don't assume bash when displaying interactive CLI prompts #21839

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ This release adds an embedded SQLite database for storing metadata required by t
1. [21840](https://github.com/influxdata/influxdb/pull/21840): Run migrations on restored bolt & SQLite metadata databases as part of the restore process.
1. [21844](https://github.com/influxdata/influxdb/pull/21844): Upgrade to latest version of `influxdata/cron` so that tasks can be created with interval of `every: 1w`.
1. [21849](https://github.com/influxdata/influxdb/pull/21849): Specify which fields are missing when rejecting an incomplete onboarding request.
1. [21839](https://github.com/influxdata/influxdb/pull/21839): Fix display and parsing of `influxd upgrade` CLI prompts in PowerShell.

## v2.0.7 [2021-06-04]

Expand Down
14 changes: 12 additions & 2 deletions cmd/influxd/launcher/launcher_helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -482,13 +482,23 @@ func (tl *TestLauncher) TaskService(tb testing.TB) taskmodel.TaskService {

func (tl *TestLauncher) BackupService(tb testing.TB) *clibackup.Client {
tb.Helper()
return &clibackup.Client{CLI: clients.CLI{}, BackupApi: tl.APIClient(tb).BackupApi}
client := tl.APIClient(tb)
return &clibackup.Client{
CLI: clients.CLI{},
BackupApi: client.BackupApi,
HealthApi: client.HealthApi,
}
}

func (tl *TestLauncher) RestoreService(tb testing.TB) *clirestore.Client {
tb.Helper()
client := tl.APIClient(tb)
return &clirestore.Client{CLI: clients.CLI{}, RestoreApi: client.RestoreApi, OrganizationsApi: client.OrganizationsApi}
return &clirestore.Client{
CLI: clients.CLI{},
RestoreApi: client.RestoreApi,
OrganizationsApi: client.OrganizationsApi,
HealthApi: client.HealthApi,
}
}

func (tl *TestLauncher) HTTPClient(tb testing.TB) *httpc.Client {
Expand Down
18 changes: 7 additions & 11 deletions cmd/influxd/upgrade/database.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,19 +8,17 @@ import (
"path/filepath"
"strings"

"github.com/influxdata/influxdb/v2/kit/platform"

"github.com/dustin/go-humanize"
"github.com/influxdata/influx-cli/v2/clients"
"github.com/influxdata/influxdb/v2"
"github.com/influxdata/influxdb/v2/cmd/internal"
"github.com/influxdata/influxdb/v2/kit/platform"
"github.com/influxdata/influxdb/v2/pkg/fs"
"github.com/influxdata/influxdb/v2/v1/services/meta"
"github.com/tcnksm/go-input"
"go.uber.org/zap"
)

// upgradeDatabases creates databases, buckets, retention policies and shard info according to 1.x meta and copies data
func upgradeDatabases(ctx context.Context, ui *input.UI, v1 *influxDBv1, v2 *influxDBv2, opts *options, orgID platform.ID, log *zap.Logger) (map[string][]platform.ID, error) {
func upgradeDatabases(ctx context.Context, cli clients.CLI, v1 *influxDBv1, v2 *influxDBv2, opts *options, orgID platform.ID, log *zap.Logger) (map[string][]platform.ID, error) {
v1opts := opts.source
v2opts := opts.target
db2BucketIds := make(map[string][]platform.ID)
Expand All @@ -40,7 +38,7 @@ func upgradeDatabases(ctx context.Context, ui *input.UI, v1 *influxDBv1, v2 *inf
log.Info("No database found in the 1.x meta")
return db2BucketIds, nil
}
if err := checkDiskSpace(ui, opts, log); err != nil {
if err := checkDiskSpace(cli, opts, log); err != nil {
return nil, err
}

Expand Down Expand Up @@ -199,7 +197,7 @@ func upgradeDatabases(ctx context.Context, ui *input.UI, v1 *influxDBv1, v2 *inf

// checkDiskSpace ensures there is enough room at the target path to store
// a full copy of all V1 data.
func checkDiskSpace(ui *input.UI, opts *options, log *zap.Logger) error {
func checkDiskSpace(cli clients.CLI, opts *options, log *zap.Logger) error {
log.Info("Checking available disk space")

size, err := DirSize(opts.source.dataDir)
Expand Down Expand Up @@ -227,12 +225,10 @@ func checkDiskSpace(ui *input.UI, opts *options, log *zap.Logger) error {
return fmt.Errorf("not enough space on target disk of %s: need %d, available %d", v2dir, size, diskInfo.Free)
}
if !opts.force {
if confirmed := internal.GetConfirm(ui, func() string {
return fmt.Sprintf(`Proceeding will copy all V1 data to %q
if confirmed := cli.StdIO.GetConfirm(fmt.Sprintf(`Proceeding will copy all V1 data to %q
Space available: %s
Space required: %s
`, v2dir, freeBytes, requiredBytes)
}); !confirmed {
`, v2dir, freeBytes, requiredBytes)); !confirmed {
return errors.New("upgrade was canceled")
}
}
Expand Down
101 changes: 26 additions & 75 deletions cmd/influxd/upgrade/setup.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,12 @@ import (
"context"
"errors"
"fmt"
"math"
"path/filepath"
"strconv"
"time"

"github.com/influxdata/influx-cli/v2/clients"
"github.com/influxdata/influx-cli/v2/clients/setup"
"github.com/influxdata/influx-cli/v2/config"
"github.com/influxdata/influxdb/v2"
"github.com/influxdata/influxdb/v2/cmd/internal"
"github.com/tcnksm/go-input"
"go.uber.org/zap"
)

Expand All @@ -25,81 +22,35 @@ func setupAdmin(ctx context.Context, v2 *influxDBv2, req *influxdb.OnboardingReq
return res, nil
}

func onboardingRequest(ui *input.UI, options *options) (*influxdb.OnboardingRequest, error) {
if (options.force || len(options.target.password) > 0) && len(options.target.password) < internal.MinPasswordLen {
return nil, internal.ErrPasswordIsTooShort
}

req := &influxdb.OnboardingRequest{
User: options.target.userName,
Password: options.target.password,
Org: options.target.orgName,
Bucket: options.target.bucket,
RetentionPeriodSeconds: influxdb.InfiniteRetention,
Token: options.target.token,
}

if options.target.retention != "" {
dur, err := internal.RawDurationToTimeDuration(options.target.retention)
if err != nil {
return nil, err
}
secs, nanos := math.Modf(dur.Seconds())
if nanos > 0 {
return nil, fmt.Errorf("retention policy %q is too precise, must be divisible by 1s", options.target.retention)
}
req.RetentionPeriodSeconds = int64(secs)
}

if options.force {
return req, nil
}

fmt.Fprintln(ui.Writer, string(internal.PromptWithColor("Welcome to InfluxDB 2.0!", internal.ColorYellow)))
if req.User == "" {
req.User = internal.GetInput(ui, "Please type your primary username", "")
}
if req.Password == "" {
req.Password = internal.GetPassword(ui, false)
func onboardingRequest(cli clients.CLI, options *options) (*influxdb.OnboardingRequest, error) {
setupClient := setup.Client{CLI: cli}
cliReq, err := setupClient.OnboardingRequest(&setup.Params{
Username: options.target.userName,
Password: options.target.password,
AuthToken: options.target.token,
Org: options.target.orgName,
Bucket: options.target.bucket,
Retention: options.target.retention,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

I can update the description in the help-text to show examples of durations (i.e. 72h)

Force: options.force,
})
if err != nil {
return nil, err
}
if req.Org == "" {
req.Org = internal.GetInput(ui, "Please type your primary organization name", "")
req := influxdb.OnboardingRequest{
User: cliReq.Username,
Org: cliReq.Org,
Bucket: cliReq.Bucket,
}
if req.Bucket == "" {
req.Bucket = internal.GetInput(ui, "Please type your primary bucket name", "")
if cliReq.Password != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So for influxdb.OnboardingRequest we don't distinguish between nil and default-value, but for api.OnboardingRequest we do?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, a consequence of codegen vs. hand-rolled. The codegen'd models use pointer-fields for every optional field.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Talked on Slack: going to try deleting influxdb.OnboardingRequest and migrating existing code to use the codegen'd model imported from the CLI

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

req.Password = *cliReq.Password
}

// Check the initial opts instead of the req to distinguish not-set from explicit 0 over the CLI.
if options.target.retention == "" {
infiniteStr := strconv.Itoa(influxdb.InfiniteRetention)
for {
rpStr := internal.GetInput(ui,
"Please type your retention period in hours.\nOr press ENTER for infinite", infiniteStr)
rp, err := strconv.Atoi(rpStr)
if rp >= 0 && err == nil {
req.RetentionPeriodSeconds = int64((time.Duration(rp) * time.Hour).Seconds())
break
}
}
if cliReq.RetentionPeriodSeconds != nil {
req.RetentionPeriodSeconds = *cliReq.RetentionPeriodSeconds
}

if confirmed := internal.GetConfirm(ui, func() string {
rp := "infinite"
if req.RetentionPeriodSeconds > 0 {
rp = (time.Duration(req.RetentionPeriodSeconds) * time.Second).String()
}
return fmt.Sprintf(`
You have entered:
Username: %s
Organization: %s
Bucket: %s
Retention Period: %s
`, req.User, req.Org, req.Bucket, rp)
}); !confirmed {
return nil, errors.New("setup was canceled")
if cliReq.Token != nil {
req.Token = *cliReq.Token
}

return req, nil
return &req, nil
}

func saveLocalConfig(sourceOptions *optionsV1, targetOptions *optionsV2, log *zap.Logger) error {
Expand Down
17 changes: 8 additions & 9 deletions cmd/influxd/upgrade/upgrade.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,16 @@ import (
"path/filepath"
"strings"

"github.com/influxdata/influxdb/v2/kit/platform"

"github.com/influxdata/influx-cli/v2/clients"
"github.com/influxdata/influx-cli/v2/pkg/stdio"
"github.com/influxdata/influxdb/v2"
"github.com/influxdata/influxdb/v2/authorization"
"github.com/influxdata/influxdb/v2/bolt"
"github.com/influxdata/influxdb/v2/dbrp"
"github.com/influxdata/influxdb/v2/internal/fs"
"github.com/influxdata/influxdb/v2/kit/cli"
"github.com/influxdata/influxdb/v2/kit/metric"
"github.com/influxdata/influxdb/v2/kit/platform"
"github.com/influxdata/influxdb/v2/kit/prom"
"github.com/influxdata/influxdb/v2/kv"
"github.com/influxdata/influxdb/v2/kv/migration"
Expand All @@ -31,7 +32,6 @@ import (
"github.com/influxdata/influxdb/v2/v1/services/meta/filestore"
"github.com/spf13/cobra"
"github.com/spf13/viper"
"github.com/tcnksm/go-input"
"go.uber.org/zap"
"go.uber.org/zap/zapcore"
)
Expand Down Expand Up @@ -155,8 +155,7 @@ func NewCommand(ctx context.Context, v *viper.Viper) (*cobra.Command, error) {
if err != nil {
return err
}
ui := &input.UI{Writer: cmd.OutOrStdout(), Reader: cmd.InOrStdin()}
return runUpgradeE(ctx, ui, options, logger)
return runUpgradeE(ctx, clients.CLI{StdIO: stdio.TerminalStdio}, options, logger)
},
Args: cobra.NoArgs,
}
Expand Down Expand Up @@ -234,7 +233,7 @@ func NewCommand(ctx context.Context, v *viper.Viper) (*cobra.Command, error) {
DestP: &options.target.retention,
Flag: "retention",
Default: "",
Desc: "optional: duration bucket will retain data. 0 is infinite. The default is 0.",
Desc: "optional: duration bucket will retain data (i.e '1w' or '72h'). Default is infinite.",
Short: 'r',
},
{
Expand Down Expand Up @@ -351,7 +350,7 @@ func buildLogger(options *logOptions, verbose bool) (*zap.Logger, error) {
return log, nil
}

func runUpgradeE(ctx context.Context, ui *input.UI, options *options, log *zap.Logger) error {
func runUpgradeE(ctx context.Context, cli clients.CLI, options *options, log *zap.Logger) error {
if options.source.configFile != "" && options.source.dbDir != "" {
return errors.New("only one of --v1-dir or --config-file may be specified")
}
Expand Down Expand Up @@ -445,7 +444,7 @@ func runUpgradeE(ctx context.Context, ui *input.UI, options *options, log *zap.L
return errors.New("InfluxDB has been already set up")
}

req, err := onboardingRequest(ui, options)
req, err := onboardingRequest(cli, options)
if err != nil {
return err
}
Expand All @@ -463,7 +462,7 @@ func runUpgradeE(ctx context.Context, ui *input.UI, options *options, log *zap.L
return err
}

db2BucketIds, err := upgradeDatabases(ctx, ui, v1, v2, options, or.Org.ID, log)
db2BucketIds, err := upgradeDatabases(ctx, cli, v1, v2, options, or.Org.ID, log)
if err != nil {
// remove all files
log.Error("Database upgrade error, removing data", zap.Error(err))
Expand Down
7 changes: 2 additions & 5 deletions cmd/influxd/upgrade/upgrade_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package upgrade

import (
"bytes"
"context"
"fmt"
"io/ioutil"
Expand All @@ -13,6 +12,7 @@ import (
"github.com/BurntSushi/toml"
"github.com/dustin/go-humanize"
"github.com/google/go-cmp/cmp"
"github.com/influxdata/influx-cli/v2/clients"
"github.com/influxdata/influxdb/v2"
"github.com/influxdata/influxdb/v2/bolt"
"github.com/influxdata/influxdb/v2/cmd/influxd/launcher"
Expand All @@ -22,7 +22,6 @@ import (
"github.com/spf13/cobra"
"github.com/spf13/viper"
"github.com/stretchr/testify/require"
"github.com/tcnksm/go-input"
"go.uber.org/zap"
"go.uber.org/zap/zaptest"
)
Expand Down Expand Up @@ -225,10 +224,8 @@ func TestUpgradeRealDB(t *testing.T) {
}

opts := &options{source: *v1opts, target: *v2opts, force: true}
ui := &input.UI{Writer: &bytes.Buffer{}, Reader: &bytes.Buffer{}}

log := zaptest.NewLogger(t, zaptest.Level(zap.InfoLevel))
err = runUpgradeE(ctx, ui, opts, log)
err = runUpgradeE(ctx, clients.CLI{}, opts, log)
require.NoError(t, err)

v := viper.New()
Expand Down
Loading