-
Notifications
You must be signed in to change notification settings - Fork 115
Fix cluster.put_component_template API #5370
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
Conversation
|
Following you can find the validation changes against the target branch for the APIs.
You can validate these APIs yourself by using the |
nielsbauman
left a comment
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.
I reverted your commit and pushed some other changes; I hope you don't mind :). My intention with my commit is to indicate what the spec should look like for these changes. I'll let it up to you if you want to make code style changes, extract parts, etc. I ran make contrib right before pushing, so I'm hoping the generated files should be ok like this. Let me know what you think!
| import { Metadata, Name, VersionNumber } from '@_types/common' | ||
| import { Duration } from '@_types/Time' | ||
| import { IndexState } from '@indices/_types/IndexState' | ||
| import { IndexTemplateMapping } from '@indices/put_index_template/IndicesPutIndexTemplateRequest' |
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.
This import isn't particularly pretty. I'll leave it up to you whether this is fine or if we should extract the IndexTemplateMapping class to a dedicated file. If we're extracting it (which I'm in favor of), we should probably also rename it.
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.
Yes, we should move to a shared place. Options include:
specification/indices/_typesspecification/cluster/_typesspecification/_types
Not sure which is best. Probably cluster since it's where Template lives in the Elasticsearch code. Unfortunately, moving a class is a breaking change, so that will be 9.3 only.
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.
Yep, moving it to specification/cluster/_types works for me 👍. I'm having trouble coming up with a good name though... IndexTemplate is already used for retrieving templates. Is there some kind of naming convention for types that are used for PUT requests?
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.
No naming convention, I'm not sure what to use. Is keeping IndexTemplateMapping that bad? It will reduce breakage for clients that don't care about the namespace of types.
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.
I mean, I don't think the name itself really makes any sense, but if it avoids breakage, I'm fine with leaving it as is. Then let's just stick to moving the class. Could you take care of that perhaps? 🙏
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.
Sure! So I did it locally, which made me realize that we already have a case of an endpoint doing this:
So let's merge without doing this for now, and I'll ask to @l-trotta what's the best thing to do here, as the location of types mostly affects the Java client.
specification/cluster/put_component_template/ClusterPutComponentTemplateRequest.ts
Show resolved
Hide resolved
specification/indices/put_index_template/IndicesPutIndexTemplateRequest.ts
Show resolved
Hide resolved
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.
Thank you for this great work, and sorry for the huge delay.
There's one failing test left for cluster.put_component_template:
// Test file: /test/free/cluster.component_template/10_basic.yml
// Test name: Add data stream lifecycle with downsampling
import { expectAssignable } from 'tsd'
import * as T from '../types'
expectAssignable<T.ClusterPutComponentTemplateRequest>({
"body": {
"template": {
"lifecycle": {
"data_retention": "10d",
"downsampling": [
{
"after": "1d",
"fixed_interval": "5m"
}
],
"downsampling_method": "last_value"
}
}
},
"name": "test-lifecycle"
})The issue is on line 12:
Property 'rounds' is missing in type '{ after: string; fixed_interval: string; }[]' but required in type 'IndicesDataStreamLifecycleDownsampling'.
Also, are we missing created_date and modified_date in the cluster.put_component_template body? Server code
specification/cluster/put_component_template/ClusterPutComponentTemplateRequest.ts
Show resolved
Hide resolved
specification/indices/put_index_template/IndicesPutIndexTemplateRequest.ts
Outdated
Show resolved
Hide resolved
Nope, we don't allow specifying those in the API: https://github.com/elastic/elasticsearch/blob/7cb125be9e971dee0fbd8b0c8d2d0d796b126d39/server/src/main/java/org/elasticsearch/action/admin/indices/template/put/PutComponentTemplateAction.java#L82-L93 |
|
I noticed another API that used |
| * @availability stack since=8.11.0 stability=stable | ||
| * @availability serverless stability=stable | ||
| */ | ||
| lifecycle?: DataStreamLifecycle |
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.
Actually, this lifecycle field in the template object can also explicitly be set to null, and the same holds for all the fields inside DataStreamLifecycle. Do you want to address that in this PR or a follow-up one?
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.
Happy to do this as a follow-up, as long as this is before 9.3, to avoid breaking those APIs twice.
| } | ||
| /** @codegen_name index_template */ | ||
| body?: IndexTemplate | ||
| body?: IndexTemplateMapping |
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.
This causes two new failures with composed_of that indices.simulate_index_template supports. Here's one:
// Test file: /test/free/indices.simulate_index_template/10_basic.yml
// Test name: Simulate index template specifying a new template
import { expectAssignable } from 'tsd'
import * as T from '../types'
expectAssignable<T.IndicesSimulateIndexTemplateRequest>({
"body": {
"composed_of": [
"ct"
],
"index_patterns": "test*",
"priority": 15,
"template": {
"aliases": {
"test_alias": {}
},
"settings": {
"index": {
"blocks": {
"write": true
}
}
}
}
},
"name": "test"
})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.
It looks like indices.simulate_index_template accepts a ComposableIndexTemplate object, while cluster.put_component_template accepts a ComponentTemplate.
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.
Ahh yeah I was mistaken indeed. The current value of IndexTemplate is not really correct, because it contains things like created_date_millis which you're not allowed to specify in the request, but it's not a disaster. I'll remove that commit, thanks for flagging.
| * @availability stack since=8.11.0 stability=stable | ||
| * @availability serverless stability=stable | ||
| */ | ||
| lifecycle?: DataStreamLifecycle |
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.
Happy to do this as a follow-up, as long as this is before 9.3, to avoid breaking those APIs twice.
| import { Metadata, Name, VersionNumber } from '@_types/common' | ||
| import { Duration } from '@_types/Time' | ||
| import { IndexState } from '@indices/_types/IndexState' | ||
| import { IndexTemplateMapping } from '@indices/put_index_template/IndicesPutIndexTemplateRequest' |
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.
No naming convention, I'm not sure what to use. Is keeping IndexTemplateMapping that bad? It will reduce breakage for clients that don't care about the namespace of types.
…ateRequest`" This reverts commit 85a3def.
|
Thanks for updating the branch @gmarouli! We have one failure left related to downsampling, with |
|
Thanks! We merged this one, which indeed fixed the issue (and fixed three other APIs along the way!) The only item left is #5370 (comment) |
|
The backport to To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-8.19 8.19
# Navigate to the new working tree
cd .worktrees/backport-8.19
# Create a new branch
git switch --create backport-5370-to-8.19
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 1cd36d69ee3f8e4b113a93dcc43092859e4e5cee
# Push it to GitHub
git push --set-upstream origin backport-5370-to-8.19
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-8.19Then, create a pull request where the |
|
The backport to To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-9.0 9.0
# Navigate to the new working tree
cd .worktrees/backport-9.0
# Create a new branch
git switch --create backport-5370-to-9.0
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 1cd36d69ee3f8e4b113a93dcc43092859e4e5cee
# Push it to GitHub
git push --set-upstream origin backport-5370-to-9.0
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-9.0Then, create a pull request where the |
|
The backport to To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-9.1 9.1
# Navigate to the new working tree
cd .worktrees/backport-9.1
# Create a new branch
git switch --create backport-5370-to-9.1
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 1cd36d69ee3f8e4b113a93dcc43092859e4e5cee
# Push it to GitHub
git push --set-upstream origin backport-5370-to-9.1
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-9.1Then, create a pull request where the |
* Fix cluster.put_component_template API * Revert "Fix cluster.put_component_template API" This reverts commit 69c9eee. * Properly use `DataStreamOptions` and fix `cluster.put_component_template` * Also use `IndexTemplateMapping` in `IndicesSimulateIndexTemplateRequest` * Allow `null` for `data_stream_options` * Revert "Also use `IndexTemplateMapping` in `IndicesSimulateIndexTemplateRequest`" This reverts commit 85a3def. --------- Co-authored-by: Niels Bauman <[email protected]> Co-authored-by: Mary Gouseti <[email protected]> (cherry picked from commit 1cd36d6) # Conflicts: # output/openapi/elasticsearch-openapi.json # output/openapi/elasticsearch-serverless-openapi.json # output/schema/schema.json
* Fix cluster.put_component_template API * Revert "Fix cluster.put_component_template API" This reverts commit 69c9eee. * Properly use `DataStreamOptions` and fix `cluster.put_component_template` * Also use `IndexTemplateMapping` in `IndicesSimulateIndexTemplateRequest` * Allow `null` for `data_stream_options` * Revert "Also use `IndexTemplateMapping` in `IndicesSimulateIndexTemplateRequest`" This reverts commit 85a3def. --------- Co-authored-by: Niels Bauman <[email protected]> Co-authored-by: Mary Gouseti <[email protected]> (cherry picked from commit 1cd36d6) # Conflicts: # output/openapi/elasticsearch-openapi.json # output/openapi/elasticsearch-serverless-openapi.json # output/schema/schema.json
💔 Some backports could not be created
Manual backportTo create the backport manually run: Questions ?Please refer to the Backport tool documentation |
|
8.19 backport currently blocked on #5807 |
I believe this was added in elastic/elasticsearch#117357, and the YAML tests added in elastic/elasticsearch#117357 helped surface the issue in the specification validation.
I'd be happy to have a better description.