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

subcommand: cluster update #296

Merged
merged 4 commits into from
Sep 1, 2020
Merged

subcommand: cluster update #296

merged 4 commits into from
Sep 1, 2020

Conversation

bonifaido
Copy link
Member

@bonifaido bonifaido commented Aug 31, 2020

Signed-off-by: Nandor Kracser [email protected]

Q A
Bug fix? no
New feature? yes
API breaks? no
Deprecations? no
Related tickets N/A
License Apache 2.0

What's in this PR?

cluster update subcommand.

Why?

To be able to use the new endpoint from the CLI, introduced in banzaicloud/pipeline#3094

Additional context

Example:

banzai cluster update <<(echo '{"version":"1.17"}') --cluster-name update-test

Checklist

  • Code meets the Developer Guide
  • User guide and development docs updated (if needed)

@bonifaido bonifaido self-assigned this Aug 31, 2020
Signed-off-by: Nandor Kracser <[email protected]>
@bonifaido bonifaido requested review from pregnor and orymate August 31, 2020 13:02
@bonifaido bonifaido marked this pull request as ready for review August 31, 2020 13:02
Makefile Outdated Show resolved Hide resolved
internal/cli/command/cluster/update.go Outdated Show resolved Hide resolved
internal/cli/command/cluster/update.go Outdated Show resolved Hide resolved
@bonifaido bonifaido marked this pull request as draft September 1, 2020 08:06
Signed-off-by: Nandor Kracser <[email protected]>
@bonifaido bonifaido marked this pull request as ready for review September 1, 2020 10:35
@bonifaido bonifaido requested a review from orymate September 1, 2020 12:23
@bonifaido bonifaido merged commit b0dde23 into master Sep 1, 2020
@bonifaido bonifaido deleted the cluster-update branch September 1, 2020 13:34
Copy link
Member

@pregnor pregnor left a comment

Choose a reason for hiding this comment

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

Kinda late to the party, but left a couple notes for the future


flags.StringVarP(&options.file, "file", "f", "", "Cluster update descriptor file")
flags.BoolVarP(&options.wait, "wait", "w", false, "Wait for cluster update")
flags.IntVarP(&options.interval, "interval", "i", 10, "Interval in seconds for polling cluster status")
Copy link
Member

@pregnor pregnor Sep 1, 2020

Choose a reason for hiding this comment

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

Note: I might have suggested a cluster-name positional argument here similarly to how banzai cluster get and banzai cluster nodepool update is implemented.

Also I'm not sure whether we expect future updates to contain more parameters but as long as only a version is expected I feel like the descriptor file seems to be an overkill, I would be glad to specify the version with a flag or interactively - of which the latter is implemented in this PR.

Copy link
Member

Choose a reason for hiding this comment

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

There are a bunch of other parameters that we will have to support eventually

Copy link
Member Author

Choose a reason for hiding this comment

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

	options.Context = clustercontext.NewClusterContext(cmd, banzaiCli, "update")

"installs" these flags to the subcommand automatically.

Copy link
Member

Choose a reason for hiding this comment

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

Regarding the version flag: that makes sense, it's good this way then.

Regarding the positional parameter: yes, I checked that, what I meant by my first paragraph was that the current implementation seems to support only explicit --cluster-name <name> or --cluster-id <id> calls, but the following would be the most intuitive for me:
banzai cluster update my-little-cluster --file ~/banzai-cluster-update.yaml
instead of
banzai cluster update --cluster-name my-little-cluster --file ~/banzai-cluster-update.yaml
similarly to other commands of ours such as cluster get and cluster nodepool update.
It's a small thing and can be added later, it just makes the CLI more consistent IMO.

pregnor_41568:[email protected]/Users/pregnor/development/src/github.com/banzaicloud/banzai-cli (master u=)
> build/banzai cluster update --help

# Begin: 2020-09-01T16:36:08.121+02:00

Update cluster based on json stdin or interactive session

Usage:
  banzai cluster update [flags]

Aliases:
  update, u

Flags:
      --cluster int32         ID of cluster to update
      --cluster-name string   Name of cluster to update
  -f, --file string           Cluster update descriptor file
  -h, --help                  help for update
  -i, --interval int          Interval in seconds for polling cluster status (default 10)
  -w, --wait                  Wait for cluster update

Global Flags:
      --color                use colors on non-tty outputs
      --config string        config file (default is $BANZAICONFIG or $HOME/.banzai/config.yaml)
      --interactive          ask questions interactively even if stdin or stdout is non-tty
      --no-color             never display color output
      --no-interactive       never ask questions interactively
      --organization int32   organization id
  -o, --output string        output format (default|yaml|json) (default "default")
      --verbose              more verbose output

# End: 2020-09-01T16:36:08.223+02:00
# Duration: 102ms
# Exit code: 0
pregnor_41568:[email protected]/Users/pregnor/development/src/github.com/banzaicloud/banzai-cli (master u=)
> build/banzai cluster get --help

# Begin: 2020-09-01T16:38:51.385+02:00

Get cluster details

Usage:
  banzai cluster get [--cluster=ID | [--cluster-name=]NAME] [flags]

Aliases:
  get, g, show

Flags:
      --cluster int32         ID of cluster to get
      --cluster-name string   Name of cluster to get
  -h, --help                  help for get

Global Flags:
      --color                use colors on non-tty outputs
      --config string        config file (default is $BANZAICONFIG or $HOME/.banzai/config.yaml)
      --interactive          ask questions interactively even if stdin or stdout is non-tty
      --no-color             never display color output
      --no-interactive       never ask questions interactively
      --organization int32   organization id
  -o, --output string        output format (default|yaml|json) (default "default")
      --verbose              more verbose output

# End: 2020-09-01T16:38:51.490+02:00
# Duration: 105ms
# Exit code: 0
pregnor_41568:[email protected]/Users/pregnor/development/src/github.com/banzaicloud/banzai-cli (master u=)
> build/banzai cluster nodepool update --help

# Begin: 2020-09-01T16:39:16.028+02:00

Update a node pool (and related subcommands)

Usage:
  banzai cluster nodepool update [NAME] [flags]
  banzai cluster nodepool update [command]

Aliases:
  update, u, upgrade

Available Commands:
  cancel      Cancel a node pool update
  tail        Tail a node pool update

Flags:
      --cluster int32         ID of cluster to update
      --cluster-name string   Name of cluster to update
  -f, --file string           Node pool descriptor file
  -h, --help                  help for update

Global Flags:
      --color                use colors on non-tty outputs
      --config string        config file (default is $BANZAICONFIG or $HOME/.banzai/config.yaml)
      --interactive          ask questions interactively even if stdin or stdout is non-tty
      --no-color             never display color output
      --no-interactive       never ask questions interactively
      --organization int32   organization id
  -o, --output string        output format (default|yaml|json) (default "default")
      --verbose              more verbose output

Use "banzai cluster nodepool update [command] --help" for more information about a command.

# End: 2020-09-01T16:39:16.073+02:00
# Duration: 45ms
# Exit code: 0

Copy link
Contributor

Choose a reason for hiding this comment

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

speaking about consistency, we should also consider that we couldn't update a cluster named 'tail' or 'cancel' this way :)

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.

4 participants