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.

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(cmd/influx): fix long startup when running influx help and other similar commands. #15777

Merged
merged 1 commit into from
Nov 6, 2019

Conversation

fchikwekwe
Copy link
Contributor

@fchikwekwe fchikwekwe commented Nov 6, 2019

Closes #15441

Describe your proposed changes here.

  • CHANGELOG.md updated with a link to the PR (not the Issue)
  • Well-formatted commit messages
  • Rebased/mergeable
  • Tests pass
  • http/swagger.yml updated (if modified Go structs or API)
  • Documentation updated or issue created (provide link to issue/pr)
  • Signed CLA (if not already signed)

@fchikwekwe fchikwekwe force-pushed the fix/long-startup branch 3 times, most recently from 052240e to f287c30 Compare November 6, 2019 01:26
Copy link
Contributor

@nathanielc nathanielc left a comment

Choose a reason for hiding this comment

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

Perfect simple solution. Thanks!

platform "github.com/influxdata/influxdb"
_ "github.com/influxdata/influxdb/query/builtin"

_ "github.com/influxdata/influxdb/query/stdlib" // Import the stdlib
"github.com/spf13/cobra"
"github.com/spf13/viper"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you remove the extra blank lines here and below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, no problem.

@fchikwekwe fchikwekwe marked this pull request as ready for review November 6, 2019 17:47
Copy link
Contributor

@mark-rushakoff mark-rushakoff left a comment

Choose a reason for hiding this comment

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

The changes look correct, but please move the FinalizeBuiltIns calls after the flag validation.

@@ -4,9 +4,12 @@ import (
"context"
"fmt"

"github.com/influxdata/flux"

Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this blank line. Our convention is to group all non-standard-library imports together.

@@ -41,6 +42,8 @@ func init() {
}

func replF(cmd *cobra.Command, args []string) error {
flux.FinalizeBuiltIns()
Copy link
Contributor

Choose a reason for hiding this comment

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

This should move after the flag validation and before the call to getFluxREPL -- it would be frustrating to spend the multiple-second wait only to find out that you mistyped an org ID.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that is true. Changing it now.

@@ -40,6 +43,8 @@ func init() {
}

func fluxQueryF(cmd *cobra.Command, args []string) error {
flux.FinalizeBuiltIns()
Copy link
Contributor

Choose a reason for hiding this comment

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

This call should also move after flag validation and before getFluxREPL.

Copy link
Contributor

@mark-rushakoff mark-rushakoff left a comment

Choose a reason for hiding this comment

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

Thanks for making the changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Long startup time for influx CLI since introducing Flux 0.42
3 participants