-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Upgrade to go 1.10 #792
Upgrade to go 1.10 #792
Conversation
vprithvi
commented
Apr 27, 2018
- remove redundant newline in Fprintln arg list to fix build
a306697
to
baafcdb
Compare
cmd/env/command.go
Outdated
@@ -40,8 +40,7 @@ their names to upper case and replacing punctuation with underscores. For exampl | |||
--cassandra.connections-per-host CASSANDRA_CONNECTIONS_PER_HOST | |||
--metrics-backend METRICS_BACKEND | |||
|
|||
The following configuration options are only available via environment variables: | |||
`) | |||
The following configuration options are only available via environment variables:`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why? The formatting is better with a blank line.
$ go run ./cmd/query/main.go env
All command line options can be provided via environment variables by converting
their names to upper case and replacing punctuation with underscores. For example,
command line option environment variable
------------------------------------------------------------------
--cassandra.connections-per-host CASSANDRA_CONNECTIONS_PER_HOST
--metrics-backend METRICS_BACKEND
The following configuration options are only available via environment variables:
SPAN_STORAGE_TYPE string The type of backend (cassandra, elasticsearch, memory) used for trace storage. (default "cassandra")
DEPENDENCY_STORAGE_TYPE string The type of backend used for service dependencies storage. (default "${SPAN_STORAGE}")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the additional newline make a difference? (The error says it's redundant)
The build failed with the following:
# github.com/jaegertracing/jaeger/cmd/env
cmd/env/command.go:34: Fprintln arg list ends with redundant newline
make: *** [test] Error 2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then I would add + "\n"
before paren. Not having new-line makes display less readable
cmd/env/command.go
Outdated
@@ -40,8 +40,7 @@ their names to upper case and replacing punctuation with underscores. For exampl | |||
--cassandra.connections-per-host CASSANDRA_CONNECTIONS_PER_HOST | |||
--metrics-backend METRICS_BACKEND | |||
|
|||
The following configuration options are only available via environment variables: | |||
`) | |||
The following configuration options are only available via environment variables:\n`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are you sure \n
is actually interpreted when in back-tick string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question, I don't expect it to be, but let me verify/ replace with something that preserves formatting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just run the cmd and see the output: go run ./cmd/query/main.go env
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
\n will be interpreted as the literal backslash and character n since it is raw string literal.
https://golang.org/ref/spec#String_literals
I would suggest inserting fmt.Fprint(cmd.OutOrStdout(), "\n") after Fprintln, if it has to be changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the complication? My original suggestion was to concatenate with +"\n"
a0e7724
to
b55638d
Compare
- remove redundant newline in Fprintln arg list to fix build Signed-off-by: Prithvi Raj <p.r@uber.com>
b55638d
to
684a086
Compare
* master: (38 commits) Preparing release 1.5.0 (jaegertracing#847) Add bounds to memory storage (jaegertracing#845) Add metric for debug traces (jaegertracing#796) Change metrics naming scheme (jaegertracing#776) Bump gocql version (jaegertracing#829) Remove ParentSpanID from domain model (jaegertracing#831) Make gas run quiet (jaegertracing#838) Revert "Make gas run quite" Revert "Install gas from install-ci" Install gas from install-ci Make gas run quite Add 'gas' for security problems scanning (jaegertracing#830) Add ability to adjust static sampling probabilities per operation (jaegertracing#827) Support log-level flag on agent (jaegertracing#828) Remove unused function (jaegertracing#822) Add healthcheck to standalone (jaegertracing#784) Do not use KeyValue fields directly and use KeyValues as decorator only (jaegertracing#810) Add ContaAzul to the adopters list (jaegertracing#806) Add ISSUE_TEMPLATE and PULL_REQUEST_TEMPLATE (jaegertracing#805) Upgrade to go 1.10 (jaegertracing#792) ... # Conflicts: # cmd/agent/app/builder.go # cmd/collector/main.go # cmd/query/main.go # cmd/standalone/main.go