Skip to content
This repository has been archived by the owner on Oct 24, 2023. It is now read-only.

feat: add human-readable output option to "get-versions" command #527

Merged
merged 8 commits into from
Feb 20, 2019

Conversation

mboersma
Copy link
Member

@mboersma mboersma commented Feb 16, 2019

Reason for Change:

The aks-engine get-versions command added in #448 defaults to JSON output that is hard to read. This adds the --output=human argument which acts similarly to az aks get-versions --output table:

$ ./bin/aks-engine get-versions -o human
Version        Upgrades
1.14.0-alpha.1 
1.13.3         1.14.0-alpha.1
1.13.2         1.13.3, 1.14.0-alpha.1
1.12.5         1.13.2, 1.13.3
1.12.4         1.12.5, 1.13.2, 1.13.3
1.11.7         1.12.4, 1.12.5
1.11.6         1.11.7, 1.12.4, 1.12.5
1.10.13        1.11.6, 1.11.7
1.10.12        1.10.13, 1.11.6, 1.11.7
1.9.11         1.10.12, 1.10.13
1.9.10         1.9.11, 1.10.12, 1.10.13
1.6.9          1.9.10, 1.9.11

Issue Fixed:

Fixes #520

Requirements:

Notes:
Should we make the table output the default?

@acs-bot acs-bot added the size/L label Feb 16, 2019
@mboersma mboersma added this to In progress in backlog via automation Feb 16, 2019
@acs-bot
Copy link

acs-bot commented Feb 16, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mboersma

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@codecov
Copy link

codecov bot commented Feb 16, 2019

Codecov Report

Merging #527 into master will increase coverage by 0.07%.
The diff coverage is 84.09%.

@@            Coverage Diff             @@
##           master     #527      +/-   ##
==========================================
+ Coverage   54.58%   54.66%   +0.07%     
==========================================
  Files          97       97              
  Lines       14641    14672      +31     
==========================================
+ Hits         7992     8020      +28     
- Misses       5974     5976       +2     
- Partials      675      676       +1

@jackfrancis
Copy link
Member

lgtm

@mboersma
Copy link
Member Author

mboersma commented Feb 19, 2019

/hold

I'm adding some tests.

Should -o table be the default?

@mboersma
Copy link
Member Author

Team feedback was that -o table should be the default, so I'll make that change.

@jackfrancis
Copy link
Member

lgtm

@mboersma
Copy link
Member Author

mboersma commented Feb 19, 2019

I notice that aks-engine version has a similar argument, but it expects human or json as the value. I like things being predictable. Should we switch table to human, or vice versa?

@jackfrancis
Copy link
Member

I think human is the more general term, for example it would be inclusive of output that wasn't tabular in nature.

@mboersma
Copy link
Member Author

I changed "table" to "human" for parity with the existing aks-engine version command, and made the help messages parallel:

$ ./bin/aks-engine get-versions -h
Display supported Kubernetes versions and upgrade versions

Usage:
  aks-engine get-versions [flags]

Flags:
  -h, --help             help for get-versions
  -o, --output string    Output format. Allowed values: human, json (default "human")
      --version string   Kubernetes version (optional)
      --windows          Kubernetes cluster with Windows nodes (optional)

Global Flags:
      --debug   enable verbose debug logs
$ ./bin/aks-engine version -h
Print the version of AKS Engine

Usage:
  aks-engine version [flags]

Flags:
  -h, --help            help for version
  -o, --output string   Output format. Allowed values: human, json (default "human")

Global Flags:
      --debug   enable verbose debug logs

@mboersma mboersma changed the title feat: add table output option to "get-versions" command feat: add human-readable output option to "get-versions" command Feb 20, 2019
@mboersma
Copy link
Member Author

I'm keeping this PR on hold just so I can merge it myself and ensure the first commit doesn't become the CHANGELOG entry.

@jackfrancis
Copy link
Member

lgtm, ready for admin merge

@mboersma mboersma merged commit e559f7d into Azure:master Feb 20, 2019
backlog automation moved this from In progress to Done Feb 20, 2019
@mboersma mboersma deleted the get-versions-table-output branch February 20, 2019 18:02
sylr pushed a commit to sylr/aks-engine that referenced this pull request Feb 28, 2019
…re#527)

* refactor: move command implementation to get_versions

* feat: add table output option to "get-versions" command

* fix: orchestrators command needs defaults

* refactor: use fmt.Errorf

* test: add cases for json, table, and invalid output formats

* feat: make table output the default for "get-versions"

* refactor: rename "table" to "human" and sync with versions command

* refactor: unify outputFormatOptions and error messages
juhacket pushed a commit to juhacket/aks-engine that referenced this pull request Mar 14, 2019
…re#527)

* refactor: move command implementation to get_versions

* feat: add table output option to "get-versions" command

* fix: orchestrators command needs defaults

* refactor: use fmt.Errorf

* test: add cases for json, table, and invalid output formats

* feat: make table output the default for "get-versions"

* refactor: rename "table" to "human" and sync with versions command

* refactor: unify outputFormatOptions and error messages
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
backlog
  
Done
Development

Successfully merging this pull request may close these issues.

get-versions command should have a human-readable option
3 participants