lib: Usage sample for lib/env programs #1977

oncilla · 2018-10-12T14:33:20Z

fixes #1903

This change is

lukedirtwalker

Reviewed 7 of 7 files at r1.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @oncilla)

go/lib/env/sample.go, line 15 at r1 (raw file):

// limitations under the License.

// Package env contains common command line and initialization code for SCION services.

I don't think we need to repeat the package level comment. It is already in env.go that is enough.

go/lib/env/sample.go, line 30 at r1 (raw file):

// ConfigFlag adds a config flag.
func ConfigFlag() *string {

I'd probably name the file envconfig.go or config.go since I think the config part is more relevant (even though more code is for the sample, but it is the sample of the config).

go/lib/env/sample.go, line 35 at r1 (raw file):

// SampleFlag adds a sample flag.
func SampleFlag() *string {

I don't think we need to "externalize" the flags. You could also create locals var here:

var (
   config string
   sample string
)

then have a function that adds the flags

func AddConfigFlags() {
    flag.StringVar(&config, "config", "", "Service TOML config file (required)")
    flag.StringVar(&sample, "sample", "", "Filename for creating a sample config. If set, the service is not started.")
}

This way the functions below don't have to get the flags again.

We do this also for the logging (see lib/log/flags.go)

go/lib/env/sample.go, line 59 at r1 (raw file):

			return writeSample(flagSample, sampleConfig), false
		}
		fmt.Fprintln(os.Stderr, "Missing config file")

Maybe we could also print it to StdOut in this case. However we would need to be sure if the flag was set or not (maybe using https://stackoverflow.com/a/35809400).
But I don't think this is very important. Do it if you feel like it.

go/path_srv/internal/psconfig/sample_test.go, line 50 at r1 (raw file):

			"gen/ISD1/ASff00_0_110/ps1-ff00_0_110-1")
		SoMsg("LogFile correct", cfg.Logging.File.Path, ShouldEqual, "logs/ps1-ff00_0_110-1.log")
		SoMsg("LogFile correct", cfg.Logging.File.Path, ShouldEqual, "logs/ps1-ff00_0_110-1.log")

Duplicate line?

oncilla

Reviewable status: 3 of 7 files reviewed, 5 unresolved discussions (waiting on @lukedirtwalker and @oncilla)

go/path_srv/internal/psconfig/sample_test.go, line 50 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

Duplicate line?

Done.

go/lib/env/sample.go, line 15 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

I don't think we need to repeat the package level comment. It is already in env.go that is enough.

whoops copy paste fail.
Done.

go/lib/env/sample.go, line 30 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

I'd probably name the file envconfig.go or config.go since I think the config part is more relevant (even though more code is for the sample, but it is the sample of the config).

I agree sample is not optimal, how about flags.go? (also consistent with lib/log)
config.go sounds more like it would actually contain a config.

go/lib/env/sample.go, line 35 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

I don't think we need to "externalize" the flags. You could also create locals var here:
var (
   config string
   sample string
)
then have a function that adds the flags
func AddConfigFlags() {
    flag.StringVar(&config, "config", "", "Service TOML config file (required)")
    flag.StringVar(&sample, "sample", "", "Filename for creating a sample config. If set, the service is not started.")
}
This way the functions below don't have to get the flags again.

We do this also for the logging (see lib/log/flags.go)

Done.

go/lib/env/sample.go, line 59 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

Maybe we could also print it to StdOut in this case. However we would need to be sure if the flag was set or not (maybe using https://stackoverflow.com/a/35809400).
But I don't think this is very important. Do it if you feel like it.

I'm not sure what you are referring to.
Do you mean the sample config should be written to StdOut instead of StdErr?
Or do you mean sample confing should be written? In that case, flag.Usage should take care of that.

lukedirtwalker

Reviewed 4 of 4 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @lukedirtwalker)

go/lib/env/sample.go, line 30 at r1 (raw file):

Previously, Oncilla wrote…

I agree sample is not optimal, how about flags.go? (also consistent with lib/log)
config.go sounds more like it would actually contain a config.

I like it

go/lib/env/sample.go, line 59 at r1 (raw file):

Previously, Oncilla wrote…

I'm not sure what you are referring to.
Do you mean the sample config should be written to StdOut instead of StdErr?
Or do you mean sample confing should be written? In that case, flag.Usage should take care of that.

Oh sorry for the unclear comment. I meant the sample config could be printed the StdOut. I.e. If I run path_src -sample it would just print the sample config (and nothing else).

kormat

Reviewed 1 of 1 files at r3.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @oncilla)

go/lib/env/flags.go, line 31 at r3 (raw file):

func AddFlags() {
	flag.StringVar(&config, "config", "", "Service TOML config file.")
	flag.BoolVar(&sample, "help-config", false, "Write sample config file.")

"Output sample commented config file."

go/lib/env/flags.go, line 40 at r3 (raw file):

// Usage returns a usage function based on the sample config.
func Usage(sampleConfig string) func() {

This param can be dropped now.

go/lib/env/flags.go, line 42 at r3 (raw file):

func Usage(sampleConfig string) func() {
	return func() {
		fmt.Fprintf(os.Stderr, "Usage: %s -config <FILE> \n   or: %s -help-config\n\nArguments:\n",

Usage should be output to stdout in the normal case, output it to stderr if a flag error has occurred. So, maybe pass in an io.Writer as a param, and Fprintf to that.

go/lib/env/flags.go, line 53 at r3 (raw file):

// The first return value is the return code of the program. The second value
// indicates whether the program can continue with its execution or should exit.
func CheckFlags(sampleConfig string) (int, bool) {

if -help-config was provided, ignore the -config flag.

go/lib/env/flags.go, line 56 at r3 (raw file):

	if config == "" {
		if sample {
			fmt.Fprint(os.Stdout, sampleConfig)

This can be just fmt.Print(sampleConfig)

kormat

Reviewed 2 of 4 files at r2.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @oncilla)

kormat

Reviewed 1 of 7 files at r1.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @oncilla)

go/path_srv/internal/psconfig/sample.go, line 20 at r3 (raw file):

  # The ID of the service. This is used to choose the relevant portion of the
  # topology file for some services.
  ID = "ps1-ff00_0_110-1"

Let's keep things simple - ps-1.

go/path_srv/internal/psconfig/sample.go, line 23 at r3 (raw file):

  # Directory for loading AS information, certs, keys, path policy, topology.
  ConfigDir = "gen/ISD1/ASff00_0_110/ps1-ff00_0_110-1"

/etc/scion/

go/path_srv/internal/psconfig/sample.go, line 27 at r3 (raw file):

  # Topology file. If not specified, topology.json is loaded from the config
  # directory.
  # Topology = "gen/ISD1/ASff00_0_110/ps1-ff00_0_110-1/topology.json"

/etc/scion/topology.json

go/path_srv/internal/psconfig/sample.go, line 35 at r3 (raw file):

  [logging.file]
    # Location of the logging file.
    Path = "logs/ps1-ff00_0_110-1.log"

/var/log/scion/ps-1.log

go/path_srv/internal/psconfig/sample.go, line 49 at r3 (raw file):

    # are immediately flushed. If negative, messages are never flushed
    # automatically. (default 5)
    FlushInterval = 10

I'd leave this at the default of 5.

go/path_srv/internal/psconfig/sample.go, line 52 at r3 (raw file):

  [logging.console]
    # Console logging level (trace|debug|info|warn|error|crit) (default crit)
    Level = "warn"

Ditto re: default of crit.

go/path_srv/internal/psconfig/sample.go, line 61 at r3 (raw file):

[infra]
  # Node type.
  Type = "PS"

Huh. I'm wondering why we have this. Surely the app knows its own service type..

go/path_srv/internal/psconfig/sample.go, line 67 at r3 (raw file):

  # initial trust information. If a file does not exist, it is created from the
  # initial information found under ConfigDir/certs.
  TrustDB = "gen-cache/ps1-ff00_0_110-1.trust.db"

/var/lib/scion/spki/ps-1.trust.db

go/path_srv/internal/psconfig/sample.go, line 71 at r3 (raw file):

[ps]
  # Path to the path database
  PathDB = "gen-cache/ps1-ff00_0_110-1.path.db"

/var/lib/scion/pathdb/ps-1.path.db

oncilla

Reviewable status: 0 of 7 files reviewed, 14 unresolved discussions (waiting on @kormat and @lukedirtwalker)

go/path_srv/internal/psconfig/sample.go, line 20 at r3 (raw file):

Previously, kormat (Stephen Shirley) wrote…

Let's keep things simple - ps-1.

Done.

go/path_srv/internal/psconfig/sample.go, line 23 at r3 (raw file):

Previously, kormat (Stephen Shirley) wrote…

/etc/scion/

Done.

go/path_srv/internal/psconfig/sample.go, line 27 at r3 (raw file):

Previously, kormat (Stephen Shirley) wrote…

/etc/scion/topology.json

Done.

go/path_srv/internal/psconfig/sample.go, line 35 at r3 (raw file):

Previously, kormat (Stephen Shirley) wrote…

/var/log/scion/ps-1.log

Done.

go/path_srv/internal/psconfig/sample.go, line 49 at r3 (raw file):

Previously, kormat (Stephen Shirley) wrote…

I'd leave this at the default of 5.

Done.

go/path_srv/internal/psconfig/sample.go, line 52 at r3 (raw file):

Previously, kormat (Stephen Shirley) wrote…

Ditto re: default of crit.

Done.

go/path_srv/internal/psconfig/sample.go, line 61 at r3 (raw file):

Previously, kormat (Stephen Shirley) wrote…

Huh. I'm wondering why we have this. Surely the app knows its own service type..

Not sure. Validity check maybe?

go/path_srv/internal/psconfig/sample.go, line 67 at r3 (raw file):

Previously, kormat (Stephen Shirley) wrote…

/var/lib/scion/spki/ps-1.trust.db

Done.

go/path_srv/internal/psconfig/sample.go, line 71 at r3 (raw file):

Previously, kormat (Stephen Shirley) wrote…

/var/lib/scion/pathdb/ps-1.path.db

Done.

go/lib/env/flags.go, line 31 at r3 (raw file):

Previously, kormat (Stephen Shirley) wrote…

"Output sample commented config file."

Done.

go/lib/env/flags.go, line 40 at r3 (raw file):

Previously, kormat (Stephen Shirley) wrote…

This param can be dropped now.

Done.

go/lib/env/flags.go, line 42 at r3 (raw file):

Previously, kormat (Stephen Shirley) wrote…

Usage should be output to stdout in the normal case, output it to stderr if a flag error has occurred. So, maybe pass in an io.Writer as a param, and Fprintf to that.

Done-ish.

Somehow flags is not flexible enough to have that exact behavior.
With this solution we write to:

stdout in case sample is set or -help is passed
stderr in case config flag not set or app.Initfails.

However, if flags.Parse() fails, the error is written to stderr and the usage message to stdout.

I don't see an easy way to fix, without doing our own parsing of the flags.

go/lib/env/flags.go, line 53 at r3 (raw file):

Previously, kormat (Stephen Shirley) wrote…

if -help-config was provided, ignore the -config flag.

Done.

go/lib/env/flags.go, line 56 at r3 (raw file):

Previously, kormat (Stephen Shirley) wrote…

This can be just fmt.Print(sampleConfig)

Done.

go/lib/env/sample.go, line 59 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

Oh sorry for the unclear comment. I meant the sample config could be printed the StdOut. I.e. If I run path_src -sample it would just print the sample config (and nothing else).

-sample is now a bool with the proposed behavior

kormat

Reviewed 5 of 7 files at r4.
Reviewable status: 5 of 7 files reviewed, 2 unresolved discussions (waiting on @lukedirtwalker, @kormat, and @oncilla)

go/path_srv/internal/psconfig/sample.go, line 61 at r3 (raw file):

Previously, Oncilla wrote…

Not sure. Validity check maybe?

Maybe @scrye knows?

go/sciond/internal/sdconfig/sample.go, line 60 at r4 (raw file):

[infra]
  # Node type.
  Type = "SD"

While i don't know what the Infra section is for, yet, the docs says it is for BS/CS/PS, so it shouldn't exist in the sciond config.

kormat

Reviewed 2 of 7 files at r4.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @oncilla)

kormat

Reviewed 4 of 5 files at r5.
Reviewable status: 6 of 7 files reviewed, 3 unresolved discussions (waiting on @kormat and @oncilla)

go/lib/env/flags.go, line 39 at r5 (raw file):

}

// Usage is the usage function which writes to stdout.

"Usage outputs run-time help to stdout"

oncilla

Reviewable status: 6 of 7 files reviewed, 3 unresolved discussions (waiting on @kormat and @oncilla)

go/sciond/internal/sdconfig/sample.go, line 60 at r4 (raw file):

Previously, kormat (Stephen Shirley) wrote…

While i don't know what the Infra section is for, yet, the docs says it is for BS/CS/PS, so it shouldn't exist in the sciond config.

Done.

kormat

Reviewed 1 of 5 files at r5.
Reviewable status: 6 of 7 files reviewed, 2 unresolved discussions (waiting on @kormat and @oncilla)

go/path_srv/internal/psconfig/sample.go, line 61 at r3 (raw file):

Previously, kormat (Stephen Shirley) wrote…

Maybe @scrye knows?

In the meantime, let's just drop it from the config. The field is literally never used.

oncilla

Reviewable status: 4 of 7 files reviewed, 2 unresolved discussions (waiting on @kormat)

go/lib/env/flags.go, line 39 at r5 (raw file):

Previously, kormat (Stephen Shirley) wrote…

"Usage outputs run-time help to stdout"

Done.

kormat

Reviewed 1 of 1 files at r6, 2 of 2 files at r7.
Reviewable status: complete! all files reviewed, all discussions resolved

lukedirtwalker requested changes Oct 15, 2018

View reviewed changes

oncilla force-pushed the config-help branch from 8a281f5 to d9071d9 Compare October 15, 2018 07:43

oncilla commented Oct 15, 2018

View reviewed changes

lukedirtwalker approved these changes Oct 15, 2018

View reviewed changes

kormat suggested changes Oct 15, 2018

View reviewed changes

kormat reviewed Oct 15, 2018

View reviewed changes

kormat suggested changes Oct 15, 2018

View reviewed changes

oncilla added 4 commits October 15, 2018 15:25

lib: Usage sample for lib/env programs

5acebaa

address feedback

9a1b530

update help

7317d4c

fix

a5e5d7c

oncilla force-pushed the config-help branch from dec8e78 to 4e6b7be Compare October 15, 2018 13:25

fix

d12dc4a

oncilla force-pushed the config-help branch from 4e6b7be to d12dc4a Compare October 15, 2018 13:32

oncilla commented Oct 15, 2018

View reviewed changes

kormat suggested changes Oct 15, 2018

View reviewed changes

kormat reviewed Oct 15, 2018

View reviewed changes

oncilla added 2 commits October 15, 2018 16:05

address feedback

501453f

usage to stdout, err to stderr

4578cc9

kormat suggested changes Oct 15, 2018

View reviewed changes

oncilla commented Oct 15, 2018

View reviewed changes

update

071f910

kormat suggested changes Oct 15, 2018

View reviewed changes

drop infra

7957e12

oncilla commented Oct 15, 2018

View reviewed changes

kormat approved these changes Oct 15, 2018

View reviewed changes

oncilla merged commit 754f777 into scionproto:master Oct 15, 2018

oncilla deleted the config-help branch October 15, 2018 15:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib: Usage sample for lib/env programs #1977

lib: Usage sample for lib/env programs #1977

oncilla commented Oct 12, 2018 •

edited

Loading

lukedirtwalker left a comment

oncilla left a comment

lukedirtwalker left a comment

kormat left a comment

kormat left a comment

kormat left a comment

oncilla left a comment

kormat left a comment

kormat left a comment

kormat left a comment

oncilla left a comment

kormat left a comment

oncilla left a comment

kormat left a comment

lib: Usage sample for lib/env programs #1977

lib: Usage sample for lib/env programs #1977

Conversation

oncilla commented Oct 12, 2018 • edited Loading

lukedirtwalker left a comment

Choose a reason for hiding this comment

oncilla left a comment

Choose a reason for hiding this comment

lukedirtwalker left a comment

Choose a reason for hiding this comment

kormat left a comment

Choose a reason for hiding this comment

kormat left a comment

Choose a reason for hiding this comment

kormat left a comment

Choose a reason for hiding this comment

oncilla left a comment

Choose a reason for hiding this comment

kormat left a comment

Choose a reason for hiding this comment

kormat left a comment

Choose a reason for hiding this comment

kormat left a comment

Choose a reason for hiding this comment

oncilla left a comment

Choose a reason for hiding this comment

kormat left a comment

Choose a reason for hiding this comment

oncilla left a comment

Choose a reason for hiding this comment

kormat left a comment

Choose a reason for hiding this comment

oncilla commented Oct 12, 2018 •

edited

Loading