Modify generator to get version to generate passed #5352
Conversation
ruflin
force-pushed
the
kibana-versioning
branch
from
a344306 to
eef6504
Compare
ruflin
changed the title
Having the version passed from external makes the generator more reusable and cleans up the internal code. * Modify internal dashboard version to common.Version This is a prerequisit for elastic#5328 and will make the change less complex.
ruflin
force-pushed
the
kibana-versioning
branch
from
eef6504 to
a93fda1
Compare
| if i.version.Major == 5 { | ||
| path, err = i.generate5x(commonFields) | ||
| } else { | ||
| path, err = i.generate6x(commonFields) |
There was a problem hiding this comment.
Since the version is now passed in I think we should also rename generate5x and generate6x in something more generic, like
if i.version.Major >= 6 {
path, err = i.generateMinVersion6(commonFields)
} else if i.version.Major >= 5 {
path, err = i.generateMinVersion5(commonFields)
} else {
return errors.New("version unsupported..")
}
| if err != nil { | ||
| return nil, err | ||
| return "", err |
There was a problem hiding this comment.
Since you already define the path veriable upfront you could return path, err without an explicit check here.
| @@ -65,7 +67,7 @@ func (i *IndexPatternGenerator) generate5x(fields common.Fields) (string, error) | |||
| return "", err | |||
| } | |||
|
|
|||
| file5x := filepath.Join(i.targetDir5x, i.targetFilename) | |||
| file5x := filepath.Join(i.targetDir, i.targetFilename) | |||
There was a problem hiding this comment.
We should remove line 64:
version, _ := common.NewVersion("5.0.0")
Rather use the given version, otherwise you don't really use the provided version from outside for fetching versionized information from the fields.
When generalizing this, the generate5x and generate6x methods have more overlap and might be a bit restructured.
There was a problem hiding this comment.
I definitively agree on this one and think the methods should probably be merge in the long run. But I suggest to do this in a follow up PR to not have too many changes in here.
There was a problem hiding this comment.
Agree on doing the method cleanup in a next version.
Not passing in the version here but hardcoding it, I consider to be a bug though, e.g. one could pass in 7.0.0 which might have differences in the fields.yml to 6.0.0 but would still get the values for 6.0.0 with this handling.
There was a problem hiding this comment.
I changed it to i.version. I agree with 7.0 it would not be the expected behaviour but this is not a "bug" that was introduced here but already in the code.
There was a problem hiding this comment.
The difference is that so far no version was given from the outside, but it was hardcoded inside the generator. Thus it was clear that it gets generated for those two defined versions.
This PR adds the possibility to pass in a version. The "bug" I was referring to would be to not use this version then in all places.
| for _, p := range pattern { | ||
| fmt.Fprintf(os.Stdout, "-- The index pattern was created under %v\n", p) | ||
| version5, _ := common.NewVersion("5.0.0") | ||
| version6, _ := common.NewVersion("6.0.0") |
There was a problem hiding this comment.
should this be 6.0.0 already or rather be 6.0.0-rc1?
As you mentioned in the former PR that we should take the meta information of versions into account, and there are breaking changes between pre-releases, the generator now treats 6.0.0.rc1 < 6.0.0.
There was a problem hiding this comment.
As we don't pass the version in I would stick with 6.0.0 as I don't think this is a field we should change over time with each release. Hopefully we can get rid of it completely in the long run as index pattern could be generated on request now that we have it in Golang so we would not have to pregenerate them.
| if i.version.Major >= 6 { | ||
| path, err = i.genereteMinVersion6(commonFields) | ||
| } else { | ||
| path, err = i.genereteMinVersion5(commonFields) |
There was a problem hiding this comment.
There is a typo: generate instead of generete.
| @@ -65,7 +67,7 @@ func (i *IndexPatternGenerator) generate5x(fields common.Fields) (string, error) | |||
| return "", err | |||
| } | |||
|
|
|||
| file5x := filepath.Join(i.targetDir5x, i.targetFilename) | |||
| file5x := filepath.Join(i.targetDir, i.targetFilename) | |||
There was a problem hiding this comment.
Agree on doing the method cleanup in a next version.
Not passing in the version here but hardcoding it, I consider to be a bug though, e.g. one could pass in 7.0.0 which might have differences in the fields.yml to 6.0.0 but would still get the values for 6.0.0 with this handling.
ruflin
force-pushed
the
kibana-versioning
branch
from
5970a46 to
cae4a3f
Compare
| dir = path.Join(dir, imp.version) | ||
| versionPath := "default" | ||
| if imp.version.Major == 5 { | ||
| versionPath = "5x" |
There was a problem hiding this comment.
@ruflin For when you are back from vacation, I think this should be 5.x instead of 5x. I'm going to make it 5.x in a PR, but let me know if it was 5x on purpose.
There was a problem hiding this comment.
That was definitively a typo.
It was changed to '5x', I think by mistake, in elastic#5352.
It was changed to '5x', I think by mistake, in #5352.
Having the version passed from external makes the generator more reusable and cleans up the internal code.
This is a prerequisit for #5328 and will make the change less complex.