Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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 contrib/completions/bash/runc
Original file line number Diff line number Diff line change
Expand Up @@ -760,6 +760,7 @@ _runc() {
start
state
update
version
help
h
)
Expand Down
22 changes: 4 additions & 18 deletions main.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,21 +4,11 @@ import (
"fmt"
"io"
"os"
"strings"

"github.com/Sirupsen/logrus"
"github.com/opencontainers/runtime-spec/specs-go"
"github.com/urfave/cli"
)

// version will be populated by the Makefile, read from
// VERSION file of the source code.
var version = ""

// gitCommit will be the hash that the binary was built from
// and will be populated by the Makefile
var gitCommit = ""

const (
specConfig = "config.json"
usage = `Open Container Initiative runtime
Expand Down Expand Up @@ -50,16 +40,11 @@ func main() {
app := cli.NewApp()
app.Name = "runc"
app.Usage = usage
app.Version = ""

var v []string
if version != "" {
v = append(v, version)
}
if gitCommit != "" {
v = append(v, fmt.Sprintf("commit: %s", gitCommit))
cli.VersionPrinter = func(context *cli.Context) {
printVersion(context)
}
v = append(v, fmt.Sprintf("spec: %s", specs.Version))
app.Version = strings.Join(v, "\n")
app.Flags = []cli.Flag{
cli.BoolFlag{
Name: "debug",
Expand Down Expand Up @@ -108,6 +93,7 @@ func main() {
startCommand,
stateCommand,
updateCommand,
versionCommand,
}
app.Before = func(context *cli.Context) error {
if context.GlobalBool("debug") {
Expand Down
14 changes: 14 additions & 0 deletions man/runc-version.8.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
# NAME
runc version - output the runtime version

# SYNOPSIS
runc version

Write version information to stdout and exit.

# DESCRIPTION
The version command writes version information for runc and the
supported spec release. This information is also exposed through
`runc --version`, and callers can use whichever is more convenient for
them.

1 change: 1 addition & 0 deletions man/runc.8.md
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ value for "bundle" is the current directory.
start executes the user defined process in a created container
state output the state of a container
update update container resource constraints
version output the runtime version
help, h Shows a list of commands or help for one command

# GLOBAL OPTIONS
Expand Down
2 changes: 2 additions & 0 deletions tests/integration/help.bats
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,13 @@ load helpers
[ "$status" -eq 0 ]
[[ ${lines[0]} =~ NAME:+ ]]
[[ ${lines[1]} =~ runc\ '-'\ Open\ Container\ Initiative\ runtime+ ]]
[ "${output}" = "${output/VERSION/}" ]

runc --help
[ "$status" -eq 0 ]
[[ ${lines[0]} =~ NAME:+ ]]
[[ ${lines[1]} =~ runc\ '-'\ Open\ Container\ Initiative\ runtime+ ]]
[ "${output}" = "${output/VERSION/}" ]
}

@test "runc command -h" {
Expand Down
12 changes: 10 additions & 2 deletions tests/integration/version.bats
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,18 @@

load helpers

@test "runc version" {
@test "runc --version" {
runc -v
[ "$status" -eq 0 ]
[[ ${lines[0]} =~ runc\ version\ [0-9]+\.[0-9]+\.[0-9]+ ]]
[[ ${lines[0]} =~ runc\ [0-9]+\.[0-9]+\.[0-9]+ ]]
[[ ${lines[1]} =~ commit:+ ]]
[[ ${lines[2]} =~ spec:\ [0-9]+\.[0-9]+\.[0-9]+ ]]
}

@test "runc version" {
runc version
[ "$status" -eq 0 ]
[[ ${lines[0]} =~ runc\ [0-9]+\.[0-9]+\.[0-9]+ ]]
[[ ${lines[1]} =~ commit:+ ]]
[[ ${lines[2]} =~ spec:\ [0-9]+\.[0-9]+\.[0-9]+ ]]
}
42 changes: 42 additions & 0 deletions version.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
package main

import (
"fmt"

"github.com/opencontainers/runtime-spec/specs-go"
"github.com/urfave/cli"
)

// version will be populated by the Makefile, read from
// VERSION file of the source code.
var version = ""

// gitCommit will be the hash that the binary was built from
// and will be populated by the Makefile
var gitCommit = ""

var versionCommand = cli.Command{
Name: "version",
Usage: "output the runtime version",
Description: `The version command outputs the runtime version.`,
Action: printVersion,
}

func printVersion(context *cli.Context) (err error) {
Copy link
Member

Choose a reason for hiding this comment

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

this entire function's contents are poorly written and is bad quality. No where else in our code do we use these functions on os.Stdout and all the branches with if/else are just bad. It needs to be improved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No where else in our code do we use these functions on os.Stdout…

I'm replacing the os.Stdout.WriteString calls with fmt.Print* calls.

… and all the branches with if/else are just bad.

Most of the if blocks are for error checking, and I don't know how to avoid that in Go. The only else is to cover “have/don't-have a version?”, and I think we want to cover both sides of that. Besides error checking and version presence, the only other conditional is for “have a Git commit?”, and I think we want to cover that too. Do you have any conditionals you'd like to see dropped?

if version == "" {
Copy link
Member

Choose a reason for hiding this comment

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

Why don't you just do the same logic with the slice and strings.Join so we don't have all these verbose if else

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On Tue, Aug 23, 2016 at 10:46:25AM -0700, Michael Crosby wrote:

+// VERSION file of the source code.
+var version = ""
+
+// gitCommit will be the hash that the binary was built from
+// and will be populated by the Makefile
+var gitCommit = ""
+
+var versionCommand = cli.Command{

  • Name: "version",
  • Usage: "output the runtime version",
  • Description: The version command outputs the runtime version.,
  • Action: printVersion,
    +}

+func printVersion(context *cli.Context) (err error) {

  • if version == "" {

Why don't you just do the same logic with the slice and strings.Join
so we don't have all these verbose if else

The same logic as what? This check is for “someone build this
withought ‘-X main.version=…’ like we have in the Makefile”, and I
think we want to keep a check for that case.

Copy link
Member

Choose a reason for hiding this comment

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

Like we had in the main.go before this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On Tue, Aug 23, 2016 at 11:09:30AM -0700, Michael Crosby wrote:

+// VERSION file of the source code.
+var version = ""
+
+// gitCommit will be the hash that the binary was built from
+// and will be populated by the Makefile
+var gitCommit = ""
+
+var versionCommand = cli.Command{

  • Name: "version",
  • Usage: "output the runtime version",
  • Description: The version command outputs the runtime version.,
  • Action: printVersion,
    +}

+func printVersion(context *cli.Context) (err error) {

  • if version == "" {

Like we had in the main.go before this change.

The code I removed from main.go was:

  • if version != "" {
  • v = append(v, version)
  • }
  • if gitCommit != "" {
  • v = append(v, fmt.Sprintf("commit: %s", gitCommit))
  • }
  • v = append(v, fmt.Sprintf("spec: %s", specs.Version))
  • app.Version = strings.Join(v, "\n")

That's building the output string in memory and using Join to store it
(which you need when you're using app.Version). But with
cli.VersionPrinter, I don't think we need that single-string form
anymore, and using os.Stdout.WriteString directly avoids building the
the string in memory. Do you want me to build the string in memory to
save some WriteString err checks?

Copy link
Member

@mikebrow mikebrow Sep 13, 2016

Choose a reason for hiding this comment

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

I believe that's what he's asking for, build a string in memory then output it.

_, err = fmt.Print("runc unknown\n")
} else {
_, err = fmt.Printf("runc %s\n", version)
}
if err != nil {
return err
}
if gitCommit != "" {
_, err = fmt.Printf("commit: %s\n", gitCommit)
if err != nil {
return err
}
}
_, err = fmt.Printf("spec: %s\n", specs.Version)
return err
}