Remove deprecated corpora.generator.size in favour of `corpora.gene…#639
Conversation
…rator.tot_events`
| additionalProperties: false | ||
| required: | ||
| - size | ||
| - tot_events |
There was a problem hiding this comment.
Could this be called total_events? Or it is too late to rename?
| - tot_events | |
| - total_events |
There was a problem hiding this comment.
tot_events is already part of the spec, but it's not used anywhere yet but for this PR and one in elastic-package.
if we include in the 3.0.0 we can rename taking the opportunity of the breaking change, up to you
There was a problem hiding this comment.
If this is not being used, let's rename to total_events if possible, as we are doing this a breaking change in any case.
spec/changelog.yml
Outdated
| - description: Using non-GA versions of the spec in GA packages produces a filterable validation error instead of a warning. | ||
| type: enhancement | ||
| link: https://github.com/elastic/package-spec/pull/627 | ||
| - description: Remove deprecated `corpora.generator.size` in favour of `corpora.generator.tot_events` |
There was a problem hiding this comment.
Maybe we can still include it in 3.0.0.
| description: A string describing the amount of data to generate. | ||
| type: string | ||
| example: 10MiB | ||
| deprecated: true # as for https://github.com/elastic/elastic-integration-corpus-generator-tool/pull/94 | ||
| tot_events: |
There was a problem hiding this comment.
In order to not break current packages using these variables.
Should we add a json patch to at least add size parameter for packages using spec version <3.0.0 ? cc @jsoriano
Probably, this would be a similar example:
There was a problem hiding this comment.
I haven't found any usage of these benchmark folders in the integrations repository.
Commands used in the integrations repository:
$ find . -type d -name system | grep benchmark
$ find . -type d -name benchmarkMaybe it's not needed to add a JSON Patch to re-add the size parameter.
WDYT ?
There was a problem hiding this comment.
@mrodm yes, to my knowledge we added the specs but the reference to them is still only it the specs itself and in the elastic-package repo
that's why I was positive to not deal with a migration path like you suggested
once we merge this and make part of v3 elastic/elastic-package#1491 will be green and everything will be fine
what do you think?
There was a problem hiding this comment.
Yeah, I agree that we don't need the migration path as this is not used yet.
There was a problem hiding this comment.
@mrodm yes, to my knowledge we added the specs but the reference to them is still only it the specs itself and in the
elastic-packagerepothat's why I was positive to not deal with a migration path like you suggested
Perfect, thanks for checking this out. Agreed, it's not need to add the JSON patch 👍
💚 Build Succeeded
History
cc @aspacca |
| description: A string describing the amount of data to generate. | ||
| type: string | ||
| example: 10MiB | ||
| deprecated: true # as for https://github.com/elastic/elastic-integration-corpus-generator-tool/pull/94 | ||
| tot_events: |
There was a problem hiding this comment.
@mrodm yes, to my knowledge we added the specs but the reference to them is still only it the specs itself and in the
elastic-packagerepothat's why I was positive to not deal with a migration path like you suggested
Perfect, thanks for checking this out. Agreed, it's not need to add the JSON patch 👍
Yes, we will include it in v3.0.0, that we will likely release today or tomorrow. |
…rator.tot_events`
What does this PR do?
Remove deprecated
corpora.generator.sizein favour ofcorpora.generator.tot_eventsforbenchmark/spec.ymlWhy is it important?
Corpus generator tool remove since v0.6.0
sizein favour oftotEvents: this is now reflected in thepackage-specso that we will be able to bump the library. Since it's a breaking-change it would be better to include in3.0.0Checklist
test/packagesthat prove my change is effective.spec/changelog.yml.Related issues