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

Add Elasticsearch version flag #1753

Merged
merged 3 commits into from
Aug 29, 2019

Conversation

pavolloffay
Copy link
Member

Follow up on #1690 (comment)

The elasticsearch version is by default automatically derived from ES API. I have added a flag which allows to specify the version. This can be used if for some reason the ping ES API is not available or it allows to use ES7 template with ES 6.8.x for easier migration to ES 7 (just wait for old indices to TLLout).

      --es-archive.version uint                                   The major Elasticsearch version. The not specified value enables auto-detection from Elasticsearch API.
      --es.version uint                                           The major Elasticsearch version. The not specified value enables auto-detection from Elasticsearch API.

Signed-off-by: Pavol Loffay <ploffay@redhat.com>
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
Copy link
Contributor

@objectiser objectiser left a comment

Choose a reason for hiding this comment

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

LGTM - just a couple of minor things.

@@ -1,24 +1,11 @@
// Code generated by mockery v1.0.0. DO NOT EDIT.

// Copyright (c) 2019 The Jaeger Authors.
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this removed by the autogeneration?

Copy link
Member Author

@pavolloffay pavolloffay Aug 22, 2019

Choose a reason for hiding this comment

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

yes, the fmt goal adds it back

@@ -96,6 +97,7 @@ func NewOptions(primaryNamespace string, otherNamespaces ...string) *Options {
TagDotReplacement: "@",
Enabled: true,
CreateIndexTemplates: true,
Version: 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: format

flagSet.Uint(
nsConfig.namespace+suffixVersion,
0,
"The major Elasticsearch version. The not specified value enables auto-detection from Elasticsearch API.")
Copy link
Contributor

Choose a reason for hiding this comment

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

"The not specified value ..." -> "If not specified, the value will be auto-detected from Elasticsearch"?

@@ -40,16 +36,6 @@ type esTokenPropagationTestHandler struct {
}

func (h *esTokenPropagationTestHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
// Return the elasticsearch version
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this removed because it was a temporary workaround when the version wasn't available?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes

Signed-off-by: Pavol Loffay <ploffay@redhat.com>
@pavolloffay pavolloffay changed the title Es add Elasticserach version flag Add Elasticsearch version flag Aug 29, 2019
@pavolloffay pavolloffay merged commit 6724390 into jaegertracing:master Aug 29, 2019
@pavolloffay
Copy link
Member Author

The version parameter was added to rollover in this PR #1769

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants